feat(native): Insert into bucketed but unpartitioned Hive table#25139
Conversation
prestodb-ci
requested review from
a team,
namya28 and
pramodsatya
and removed request for
a team
|
@anandamideShakyan : Thanks for this PR. Have you tried this functionality with Prestissimo ? You might need facebookincubator/velox#13283 as well for it. |
|
@aditi-pandit Sure I will add the support in Prestissimo after facebookincubator/velox#13283 is merged. |
|
@anandamideShakyan : Ther are failures in product tests. PTAL. |
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
4 times, most recently
from
0e437dd to
38805a8
Compare
I tried it on Prestissimo, with one coordinator and one worker. I created a table in hive schema and tpcds catalog using: Inserted values: Was able to see the entries on running the select query: 
|
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
38805a8 to
817f7df
Compare
| import static java.lang.Boolean.parseBoolean; | ||
| import static org.testng.Assert.assertEquals; | ||
|
|
||
| public class TestHivePartitionedInsertNative |
There was a problem hiding this comment.
Could we move these testcases to presto-tests or presto-product-tests? Ideally, we don't want to add new testcases to presto-native-tests, instead we should just extend the existing e2e tests (such as the ones added to presto-product-tests in this PR) to run with with the native query runner.
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
817f7df to
ed26ecc
Compare
|
Consider adding an example of how to use this new ability, or at least a mention that this is now possible for users to do and why it's useful (as you wrote in the Description), to the documentation. |
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
ed26ecc to
e8591d3
Compare
anandamideShakyan
changed the title
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
e8591d3 to
f68fe3d
Compare
|
@anandamideShakyan : It will be good to complete this work as it has been a long pending item. Please can you take a look at the failures. |
|
Inserts into bucketed Hive tables using the C++ (Velox) worker were failing during finishInsert with:
This happens because Presto’s Hive metadata layer assumes exactly one file per bucket per partition. The Java worker never hits this path because it always creates one file per bucket, even when a bucket receives zero rows. The Velox (C++) HiveDataSink, however, only created writers for buckets that actually received rows. When a bucket was empty, no writer → no file, causing Presto to think the bucket was missing and fail verification. This is why inserts succeeded when data happened to hit all buckets, and failed otherwise. Fix The fix ensures that Velox creates one writer (and therefore one output file) per bucket, matching Java worker behavior and Presto’s expectations. Specifically: During HiveDataSink::splitInputRowsAndEnsureWriters(), we now pre-create writers for all buckets (for each partition, if partitioned). This guarantees that every bucket produces exactly one file, even if it contains zero rows. As a result, computeFileNamesForMissingBuckets() is never triggered and finishInsert succeeds. To Do
With this fix locally, I am able to insert into bucketed hive tables with and without sidecar. I am now looking at resolving the unit test failure that came after these changes : #25115 |
|
@anandamideShakyan : Presto has a property hive.create-empty-bucket-files to control whether to create empty bucket files. Seems like this should always be false for native engine. But in any case, doesn't Presto server create the missing buckets on the co-ordinator in the TableFinish logic and not in the worker ? I feel it should be on co-ordinator in TableFinish as its only after seeing all the worker files should we know which buckets are empty. The individual worker cannot make this decision. This error seems like a local problem between Hive and Presto on the co-ordinator. Please can you recheck if something else is missing. |
|
@aditi-pandit You were right, I am able to run insert queries successfully when I set I checked the sidecar worker startup logs: It is failing at registerSidecarEndpoints(). Have I missed out any configuration that is causing the sidecar registration to fail? |
|
I was getting a SIGSEGV crash ("Error getting native plan checker response" in UI) when inserting into Hive bucketed unpartitioned tables with native sidecar enabled. After investigating with @pdabre12, we found that the native sidecar crashes at line 2228 in |
|
|
||
| assertEquals(computeActual("SELECT * from " + tableName).getRowCount(), 0); | ||
|
|
||
| // make sure that we will get one file per bucket regardless of writer count configured |
There was a problem hiding this comment.
How are you validating there is only one file ? You could use a query with $path hidden column for it. https://prestodb.io/docs/0.272/connector/hive.html#extra-hidden-columns
There was a problem hiding this comment.
$path only exposes files that contain rows. Since this test inserts only two records, multiple records could hash to the same bucket and empty bucket files would not be visible through $path. To verify that all 11 bucket files were created, we'd need to inspect the table location directly (or insert data guaranteed to populate every bucket).
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
99d12a5 to
ce1f8d6
Compare
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
ce1f8d6 to
61b78a5
Compare
|
Velox based PR review. @anandamideShakyan : Please fix the test issue found. Summary Issues Found Positive Observations |
|
@aditi-pandit I have fixed the above test issue. Another thing is that the insertion fails in native execution because C++ workers add .parquet extension to target file names (e.g., "000000_0_<'queryId'>.parquet") while Java's I've fixed this by changing the coordinator logic to extract and compare bucket numbers instead of exact file names, which works regardless of extension presence, but I'd like guidance on whether we should also align C++ to match Java's approach, or if the bucket-based matching solution is the preferred fix. |
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
c2a68e7 to
efa89ca
Compare
|
@tdcmeehan : Please can you help review this PR. |
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
efa89ca to
d78bbe5
Compare
|
I have added some checks to disable validation on connector-specific partitions which contains null buckettopartition value and fail validation. @tdcmeehan Please let me know if my changes are okay or if we can do it in a better way. |
|
@anandamideShakyan : Please can you add a release note and documentation for this issue. |
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
Please add documentation for this to the Presto documentation. Perhaps here:
As I wrote in this comment:
"Consider adding an example of how to use this new ability, or at least a mention that this is now possible for users to do and why it's useful (as you wrote in the Description), to the documentation."
Thank you for adding the release note @anandamideShakyan! |
|
@steveburnett I have added the documentation. PTAL and let me know if anything else is needed. Thanks. |
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
0b1c11d to
e36e425
Compare
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull branch, local doc build, looks good. Thanks!
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
bab5bba to
cefdf12
Compare
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Documentation removed per discussion.
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
cefdf12 to
b1183b2
Compare
anandamideShakyan
dismissed stale reviews from steveburnett and tdcmeehan
via
5a63db2
anandamideShakyan
force-pushed
the
insert-bucketed-unpar-hive
branch
from
5a63db2 to
b1183b2
Compare
Description
Addresses #25104
Currently, Presto does not support INSERT INTO operations on bucketed but unpartitioned Hive tables. This limitation originates from a hard check in HiveWriterFactory:
https://github.com/prestodb/presto/blob/master/presto-hive/src/main/java/com/facebook/presto/hive/HiveWriterFactory.java#L480
Motivation and Context
Supporting writes to bucketed unpartitioned Hive tables in Presto would improve compatibility and enhance Presto’s ability to handle modern Hive table layouts. It's a reasonable and useful feature for users who wish to leverage bucketing for performance optimizations even without partitioning.
Impact
This change would align Presto’s behavior with the broader SQL-on-Hadoop ecosystem and remove an artificial limitation that may block valid use cases — particularly in data warehousing environments where bucketing is used independently of partitioning.
Release Notes