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-32469][SQL] ApplyColumnarRulesAndInsertTransitions should be idempotent #29273
Conversation
@@ -490,8 +490,10 @@ case class ApplyColumnarRulesAndInsertTransitions(conf: SQLConf, columnarRules: | |||
// The tree feels kind of backwards | |||
// Columnar Processing will start here, so transition from row to columnar | |||
RowToColumnarExec(insertTransitions(plan)) | |||
} else { | |||
} else if (!plan.isInstanceOf[RowToColumnarExec]) { |
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 had gone down the same path at one point with similar changes, but then had to introduce a new RowToColumnarExecLike
trait so that it would also work with the GpuRowToColumnarExec
implementation that we have in our plugin.
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.
do you mean https://github.com/apache/spark/pull/29067/files#diff-3a92abd70b28650055b16d8a101a95c3R55 ?
I can wait for it to be merged, and add something similar for RowToColumnarExec
.
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.
Yes, exactly like that.
Test build #126727 has finished for PR 29273 at commit
|
Test build #126982 has finished for PR 29273 at commit
|
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.
changes lgtm.
thanks for review, merging to master! |
What changes were proposed in this pull request?
This PR makes
ApplyColumnarRulesAndInsertTransitions
idempotent (assuming the custom columnar rules are also idempotent).Why are the changes needed?
It's good hygiene to keep rules idempotent
Does this PR introduce any user-facing change?
No
How was this patch tested?
new suite