[SPARK-27782][SQL] Use '#' to mark expression id embedded in the name field of SubqueryExec operator #24652
Conversation
…name in the SubqueryExec operator.
|
Test build #105577 has finished for PR 24652 at commit
|
|
Retest this please. |
|
|
||
| -- TC 03.01 | ||
| -- explain a simple uncorelated scalar subquery | ||
| SET spark.sql.codegen.wholeStage = true; |
There was a problem hiding this comment.
So, this is a workaround to avoid SQLQueryTestSuite's codegenConfigSets combinations, right?
There was a problem hiding this comment.
@dongjoon-hyun Yes.. this is to bypass the codegenconfigsets combination.. Do you want to move this test case to a new .sql instead of modifying an existing one ?
There was a problem hiding this comment.
For this PR, it might be a simple workaround to turn off three combination of codegenConfigSets. However, this will bite us when we add a new test case later at the end of the file.
SET spark.sql.codegen.wholeStage = true;
As you needed to add a workaround, it seems that SQLQueryTestSuite is not designed to serve well this kind of use case. Please move this test coverage from scalar-subquery-predicate.sql to another test suite.
|
Test build #105783 has finished for PR 24652 at commit
|
Actually i am working on the new explain item and would like to test the new format out by using the SQLQueryTestSuite framework as much as possible as there would be so many combinations of plans. I was thinking to improve SQLQueryTestSuite to introduce a new list/map to have a list of test cases per codegen config or something in that direction. Or may be have 2 output files one for codegen on and one for codegen off. Please let me know what you think. |
|
For this specific PR, please use another test suite as I recommended before. For your suggestion, please file another JIRA because that is not relevant to this tiny PR. Your suggestion will be a massive PR, isn't it? |
| FROM t2 | ||
| WHERE t2c = t1c | ||
| GROUP BY t2c); | ||
|
|
There was a problem hiding this comment.
Ur, please revert this, too. And we need to update PR description. Especially, the following. We had better keep narrow context in this PR.
This is to help SQLQueryTestSuite anonymize these expression ids.
| } | ||
| } | ||
|
|
||
| test("SPARK-27782 scalar subquuery name should start with scalar-subquery#") { |
There was a problem hiding this comment.
subquuery -> subquery.
And, please remove SPARK-27782 since this is a new feature instead of bug. We use JIRA id for bugs in general.
| |SELECT a | ||
| |FROM l | ||
| |WHERE a = (SELECT max(c) FROM r WHERE c = 1) | ||
| """.stripMargin) |
There was a problem hiding this comment.
Since this is very short, can we have one-liner?
val df = sql("SELECT a FROM l WHERE a = (SELECT max(c) FROM r WHERE c = 1)")
|
Test build #105801 has finished for PR 24652 at commit
|
|
Test build #105804 has finished for PR 24652 at commit
|
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Merged to master!
Thank you, @dilipbiswal and @gatorsmile .
|
@dongjoon-hyun Thank you very much @gatorsmile @dongjoon-hyun |
|
You're welcome always! |
4 participants
What changes were proposed in this pull request?
This is a minor pr to use
#as a marker for expression id that is embedded in the name field of SubqueryExec operator.How was this patch tested?
Added a small test in SubquerySuite.