-
Notifications
You must be signed in to change notification settings - Fork 28.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
[SPARK-20718][SQL][followup] Fix canonicalization for HiveTableScanExec #17962
Conversation
@@ -442,4 +442,14 @@ object QueryPlan { | |||
} | |||
}.canonicalized.asInstanceOf[T] | |||
} | |||
|
|||
/** Normalize and reorder the expressions in the given sequence. */ | |||
def canonicalizeExprSeq(exprSeq: Seq[Expression], output: AttributeSeq): Seq[Expression] = { |
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: normalizePredicates
/** Normalize and reorder the expressions in the given sequence. */ | ||
def canonicalizeExprSeq(exprSeq: Seq[Expression], output: AttributeSeq): Seq[Expression] = { | ||
if (exprSeq.nonEmpty) { | ||
val normalizedExprs = QueryPlan.normalizeExprId(exprSeq.reduce(And), output) |
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: QueryPlan.normalizeExprId
-> normalizeExprId
@@ -442,4 +442,14 @@ object QueryPlan { | |||
} | |||
}.canonicalized.asInstanceOf[T] | |||
} | |||
|
|||
/** Normalize and reorder the expressions in the given sequence. */ |
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.
Where do we reorder the predicates?
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.
It links predicates with AND
and pass it to def normalizeExprId
, in which Canonicalize.execute
is called. That's where we do the reordering.
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.
Got it. Could you update the above comment, since this is a little bit confusing.
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.
OK. I'll update the comments.
LGTM cc @cloud-fan |
LGTM |
Test build #76853 has finished for PR 17962 at commit
|
Test build #76859 has finished for PR 17962 at commit
|
Test build #76861 has finished for PR 17962 at commit
|
## What changes were proposed in this pull request? Fix canonicalization for different filter orders in `HiveTableScanExec`. ## How was this patch tested? Added a new test case. Author: wangzhenhua <[email protected]> Closes #17962 from wzhfy/canonicalizeHiveTableScanExec. (cherry picked from commit 54b4f2a) Signed-off-by: Wenchen Fan <[email protected]>
thanks, merging to master/2.2! |
## What changes were proposed in this pull request? Fix canonicalization for different filter orders in `HiveTableScanExec`. ## How was this patch tested? Added a new test case. Author: wangzhenhua <[email protected]> Closes apache#17962 from wzhfy/canonicalizeHiveTableScanExec.
## What changes were proposed in this pull request? Fix canonicalization for different filter orders in `HiveTableScanExec`. ## How was this patch tested? Added a new test case. Author: wangzhenhua <[email protected]> Closes apache#17962 from wzhfy/canonicalizeHiveTableScanExec.
What changes were proposed in this pull request?
Fix canonicalization for different filter orders in
HiveTableScanExec
.How was this patch tested?
Added a new test case.