-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Support grouped execution for eligible table scans #12934
Conversation
So is this to say that we should always do grouped execution for eligible tablescans even if the nodes above it don't care about grouped execution? Can you give more context about why/when we want this behavior? |
@rschlussel : This allows partial recovery for ScanFilterProject-only queries (reading from bucketed table). Also, today reading from bucketed table with ungrouped execution is somewhat hacky, for example, it requires (https://github.com/prestodb/presto/wiki/HiveSplitSource-and-Grouped-Execution#old-days):
So popularizing grouped execution seems to be the right direction in the long-term -- i.e. the stage scheduler is aware that he is scheduling split in a bucket-aware way. |
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.
LGTM. @rschlussel do you mind also taking a look?
Also maybe explain in the commit message about the motivation (for recoverable grouped execution).
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
if (ImmutableList.of(NOT_PARTITIONED).equals(partitionHandles)) { | ||
return new GroupedExecutionProperties(false, false, ImmutableList.of(), 1, recoveryEligible); | ||
return new GroupedExecutionProperties(false, useful, ImmutableList.of(), 1, recoveryEligible); |
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.
I feel GroupedExecutionProperties#useful
should be renamed. But don't have to do it in this PR.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
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.
Please add more context to the commit message
325d20a
to
68333b1
Compare
This is controlled by session property grouped_execution_for_eligible_table_scans. This could help enable recoverable grouped execution for ScanFilterProject-only queries that read from partitioned table (for example, bucketed table in Hive).
This is controlled by session property grouped_execution_for_all_eligible_table_scans.
Related to #12124