Uh oh!
There was an error while loading. Please reload this page.
[SPARK-34079][SQL][FOLLOW-UP] Revert some changes in InjectRuntimeFilterSuite - #36361
[SPARK-34079][SQL][FOLLOW-UP] Revert some changes in InjectRuntimeFilterSuite#36361peter-toth wants to merge 3 commits into
Conversation
peter-toth
commented
Apr 26, 2022
cc @cloud-fan |
| 1 | ||
| val numBloomFilterAggs = plan.collect { | ||
| case Filter(condition, _) => condition.collect { | ||
| case subquery: org.apache.spark.sql.catalyst.expressions.ScalarSubquery |
There was a problem hiding this comment.
I like the previous code more as it precisely matches Filter and ScalarSubquery. +1 to this change.
My major concern is scalarSubqueryCTEMultiplicator. This makes the test really hard to write as we need to manually think about if scalar subqueries merging can be applied or not to the testing query.
Can we turn off this optimization completely in this test suite? We can add one more test to verify the case that scalar subqueries merging is beneficial to bloom filter join, by explicitly enabling the optimizer rule.
There was a problem hiding this comment.
Got it, let me check...
cloud-fan
commented
Apr 27, 2022
thanks, merging to master/3.3! |
…terSuite To remove unnecessary changes from `InjectRuntimeFilterSuite` after #32298. These are not needed after #34929 as the final optimized plan does'n contain any `WithCTE` nodes. No need for those changes. No. Added new test. Closes#36361 from peter-toth/SPARK-34079-multi-column-scalar-subquery-follow-up-2. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit d05e01d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
peter-toth
commented
Apr 27, 2022
Thanks @cloud-fan for the review! |
What changes were proposed in this pull request?
To remove unnecessary changes from
InjectRuntimeFilterSuiteafter #32298. These are not needed after #34929 as the final optimized plan does'n contain anyWithCTEnodes.Why are the changes needed?
No need for those changes.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added new test.