-
Notifications
You must be signed in to change notification settings - Fork 232
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
fixUpJoinConsistency rule now works when AQE is enabled #676
Conversation
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
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.
Just had one question
// since they already started to execute | ||
// since they already started to execute, but we verify that they are both on CPU or | ||
// both on GPU | ||
if (queryStages.map(isGpuQueryStage).distinct.size == 2) { |
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.
so for my edification, we can't get in this situation unless there is a bug, or perhaps a change in behavior from AQE rules proper?
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 think we could only get here due to a bug when planning query stages.
Signed-off-by: Andy Grove <andygrove@nvidia.com>
build |
Signed-off-by: Andy Grove <andygrove@nvidia.com>
build |
@abellina I was able to greatly simplify the logic in this PR. Please take a look when you can. |
* fixUpJoinConsistency rule now works with AQE Signed-off-by: Andy Grove <andygrove@nvidia.com> * Add comma to error message Signed-off-by: Andy Grove <andygrove@nvidia.com> * Improved validation checks and error messages Signed-off-by: Andy Grove <andygrove@nvidia.com> * bug fix: walk tree once to find shuffle exchanges and query stages Signed-off-by: Andy Grove <andygrove@nvidia.com> * code simplification Signed-off-by: Andy Grove <andygrove@nvidia.com>
* fixUpJoinConsistency rule now works with AQE Signed-off-by: Andy Grove <andygrove@nvidia.com> * Add comma to error message Signed-off-by: Andy Grove <andygrove@nvidia.com> * Improved validation checks and error messages Signed-off-by: Andy Grove <andygrove@nvidia.com> * bug fix: walk tree once to find shuffle exchanges and query stages Signed-off-by: Andy Grove <andygrove@nvidia.com> * code simplification Signed-off-by: Andy Grove <andygrove@nvidia.com>
* first pass at a benchmark. Float only for now. * signoff Signed-off-by: Mike Wilson <knobby@burntsheep.com> Signed-off-by: Mike Wilson <knobby@burntsheep.com>
GpuOverrides
applies afixUpJoinConsistency
rule to ensure that the inputs to aSortMergeJoin
orShuffledHashJoin
are either both on CPU, or both on GPU. We can't support a mix of CPU and GPU because the hashing algorithms are not compatible, and therefore the join would produce incorrect results.This PR adds a unit test for this rule, and also ensures that the rule is applied when AQE is enabled.
This closes #631