[SPARK-39259][SQL][FOLLOWUP] Fix source and binary incompatibilities in transformDownWithSubqueries #36765
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshRosen
changed the title
MaxGekk
approved these changes
Jun 4, 2022
Member
|
+1, LGTM. Merging to master/3.3. |
MaxGekk
pushed a commit
that referenced
this pull request
Jun 4, 2022
…in transformDownWithSubqueries ### What changes were proposed in this pull request? This is a followup to #36654. That PR modified the existing `QueryPlan.transformDownWithSubqueries` to add additional arguments for tree pattern pruning. In this PR, I roll back the change to that method's signature and instead add a new `transformDownWithSubqueriesAndPruning` method. ### Why are the changes needed? The original change breaks binary and source compatibility in Catalyst. Technically speaking, Catalyst APIs are considered internal to Spark and are subject to change between minor releases (see [source](https://github.com/apache/spark/blob/bb51add5c79558df863d37965603387d40cc4387/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/package.scala#L20-L24)), but I think it's nice to try to avoid API breakage when possible. While trying to compile some custom Catalyst code, I ran into issues when trying to call the `transformDownWithSubqueries` method without supplying a tree pattern filter condition. If I do `transformDownWithSubqueries() { f} ` then I get a compilation error. I think this is due to the first parameter group containing all default parameters. My PR's solution of adding a new `transformDownWithSubqueriesAndPruning` method solves this problem. It's also more consistent with the naming convention used for other pruning-enabled tree transformation methods. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes #36765 from JoshRosen/SPARK-39259-binary-compatibility-followup. Authored-by: Josh Rosen <[email protected]> Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit eda6c4b) Signed-off-by: Max Gekk <[email protected]>
HyukjinKwon
reviewed
Jun 4, 2022
| * first to this node, then this node's subqueries and finally this node's children. | ||
| * When the partial function does not apply to a given node, it is left unchanged. | ||
| */ | ||
| def transformDownWithSubqueriesAndPruning( |
Member
There was a problem hiding this comment.
I was about to say we shouldn't make these changes for binary compatibility for internal API (e.g., #35378) but reading the codes, it looks more like a refactoring. So LGTM from me 2.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This is a followup to #36654. That PR modified the existing
QueryPlan.transformDownWithSubqueriesto add additional arguments for tree pattern pruning.In this PR, I roll back the change to that method's signature and instead add a new
transformDownWithSubqueriesAndPruningmethod.Why are the changes needed?
The original change breaks binary and source compatibility in Catalyst. Technically speaking, Catalyst APIs are considered internal to Spark and are subject to change between minor releases (see source), but I think it's nice to try to avoid API breakage when possible.
While trying to compile some custom Catalyst code, I ran into issues when trying to call the
transformDownWithSubqueriesmethod without supplying a tree pattern filter condition. If I dotransformDownWithSubqueries() { f }then I get a compilation error. I think this is due to the first parameter group containing all default parameters.My PR's solution of adding a new
transformDownWithSubqueriesAndPruningmethod solves this problem. It's also more consistent with the naming convention used for other pruning-enabled tree transformation methods.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.