-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incorrect $bucket for mismatch bucket queries #11885
Fix incorrect $bucket for mismatch bucket queries #11885
Conversation
3580a93
to
f78d3a1
Compare
assertUpdate( | ||
"CREATE TABLE test_mismatch_bucketing32\n" + | ||
"WITH (bucket_count = 32, bucketed_by = ARRAY['key32']) AS\n" + | ||
"SELECT custkey key32, comment value32 FROM orders", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testable with smaller tables?
Like nation
with bucketing on nationkey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
assertQuery(withMismatchOptimization, query, "SELECT 130361"); | ||
assertQuery(withoutMismatchOptimization, query, "SELECT 130361"); | ||
} | ||
finally { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the try-finally isn't really needed in this class, since all the test data is ephemeral anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try-finally
15000); | ||
|
||
Session withMismatchOptimization = Session.builder(getSession()) | ||
.setSystemProperty(COLOCATED_JOIN, "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter? document why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
.build(); | ||
|
||
@Language("SQL") String query = "SELECT count(*) AS count\n" + | ||
"FROM (\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: \n
are redundant, replace with spaces so that it's more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
3ce8645
to
185deca
Compare
Hi @arhimondr, Can you please have a look? |
@@ -219,6 +219,14 @@ public ConnectorSplitSource getSplits( | |||
// sort partitions | |||
partitions = Ordering.natural().onResultOf(HivePartition::getPartitionId).reverse().sortedCopy(partitions); | |||
|
|||
if (bucketHandle.isPresent()) { | |||
if (bucketHandle.get().getReadBucketCount() > bucketHandle.get().getTableBucketCount()) { | |||
throw new TrinoException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please open a follow up PR dropping the HivePartitioningHandle#maxCompatibleBucketCount
. Currently it is effectively unused as number of read buckets higher than number of table buckets is no longer supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@oraclechang Could you please rebase and resubmit to make sure it compiles on Trunk. I will merge once the build is green. |
185deca
to
9adf088
Compare
Comparable change: prestodb/presto#12429
This change allows executing queries with the following conditions. Currently, these queries will raise an exception.
A more specific example can be seen in the
testMismatchedBucketWithBucketPredicate()
test.