-
Notifications
You must be signed in to change notification settings - Fork 28k
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-37933][SQL] Change the traversal method of V2ScanRelationPushDown push down rules #35242
Conversation
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.
Thank you for pinging me, @stczwd . It looks reasonable.
I'll leave this PR to @cloud-fan . |
@@ -222,7 +231,7 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper { | |||
Cast(aggAttribute, aggDataType) | |||
} | |||
|
|||
def applyColumnPruning(plan: LogicalPlan): LogicalPlan = plan.transform { | |||
def pushDownColumn(plan: LogicalPlan): LogicalPlan = plan.transform { |
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.
maybe pruneColumns
?
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 also think pruneColumns
or the original name applyColumnPruning
might be better.
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.
pruneColumns
sounds good to me, thanks.
@@ -308,7 +317,7 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper { | |||
case other => other | |||
} | |||
|
|||
def applyLimit(plan: LogicalPlan): LogicalPlan = plan.transform { | |||
def pushDownLimit(plan: LogicalPlan): LogicalPlan = plan.transform { |
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.
the name pushDownLimit
is same as pushDownLimit
above. Can we use a different name or at least add some docs?
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.
Hm, maybe pushdownLimits
is better.
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.
LGTM2 except two comments above.
...re/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala
Outdated
Show resolved
Hide resolved
Merged to master. |
Thanks you all, guys |
What changes were proposed in this pull request?
This pr is trying to change the traversal method of V2ScanRelationPushDown push down rules , which is more readable and easier to extend and add new rules.
Does this PR introduce any user-facing change?
No
How was this patch tested?
origin tests