[SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression #31352

imback82 · 2021-01-26T20:19:27Z

What changes were proposed in this pull request?

This PR is a follow-up to #31368 to add a test case that has a subquery with "view" in aggregate's grouping expression. The existing test tests a subquery with dataframe's temp view, so it doesn't contain a View node.

Why are the changes needed?

To increase the test coverage.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a new test.

imback82 · 2021-01-26T20:22:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/view.scala

-      Project(newOutput, child)
+  override def apply(plan: LogicalPlan): LogicalPlan = {
+    AnalysisHelper.allowInvokingTransformsInAnalyzer {
+      plan transformUp {


There is no change below this line. If you don't like the diff, I can change the PR to something like the following:

override def apply(plan: LogicalPlan): LogicalPlan = { AnalysisHelper.allowInvokingTransformsInAnalyzer { applyInternal(plan) } } private def applyInternal(plan: LogicalPlan): LogicalPlan = plan transformUp {

or I would just simply do:

AnalysisHelper.allowInvokingTransformsInAnalyzer { plan transformUp { ... }}

Thanks @HyukjinKwon for the suggestion! Since allowInvokingTransformsInAnalyzer introduces one more indentation, I had to move it to align with override, which I am not sure is the style we want to follow. Please let me know!

imback82 · 2021-01-26T20:23:04Z

sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala

+        r.write.saveAsTable("tr")
+        sql("create view vr as select * from tr")
+        checkAnswer(
+          sql("select a, (select sum(d) from vr where a = c) sum_d from l l1 group by 1, 2"),


Note that the existing test with the same query worked fine because r is a dataframe temp view, which doesn't have the View node.

dongjoon-hyun

@imback82 . Is the example correct?
Spark 3.1.1 RC1

scala> sql("create temporary view ta(a, b) as select 1, 2")

scala> sql("create temporary view tc(c, d) as select 1, 2")

scala> sql("select a, (select sum(d) from tc where a = c) sum_d from ta group by 1, 2").show
+---+-----+
|  a|sum_d|
+---+-----+
|  1|    2|
+---+-----+

scala> spark.version
res3: String = 3.1.1

Spark 3.2.0

scala> sql("create temporary view ta(a, b) as select 1, 2")

scala> sql("create temporary view tc(c, d) as select 1, 2")

scala> sql("select a, (select sum(d) from tc where a = c) sum_d from ta group by 1, 2").show
+---+-----+
|  a|sum_d|
+---+-----+
|  1|    2|
+---+-----+

scala> spark.version
res3: String = 3.2.0-SNAPSHOT

imback82 · 2021-01-26T22:01:53Z

Thanks @dongjoon-hyun for checking. Let me double-check and get back to you.

dongjoon-hyun · 2021-01-26T23:35:54Z

Thanks. Maybe, do I need to use some configuration?

SparkQA · 2021-01-27T00:58:33Z

Test build #134519 has finished for PR 31352 at commit 651a2ec.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2021-01-27T01:16:16Z

Ah, it's failing only under tests:

  protected def assertNotAnalysisRule(): Unit = {
    if (Utils.isTesting &&
        AnalysisHelper.inAnalyzer.get > 0 &&
        AnalysisHelper.resolveOperatorDepth.get == 0) {
      throw new RuntimeException("This method should not be called in the analyzer")
    }
  }

Utils.isTesting is true if SPARK_TESTING env variable is set.

Is this OK?

imback82 · 2021-01-27T02:44:17Z

@cloud-fan Is this an issue if it fails only under tests (assertNotAnalysisRule() check), or no?

HyukjinKwon · 2021-01-27T04:23:12Z

Let's remove the regression mentioned in PR description. Seems like it's not a regression.

imback82 · 2021-01-27T04:38:40Z

Let's remove the regression mentioned in PR description. Seems like it's not a regression.

In Spark 3.1.1-RC, if we start spark-shell with the SPARK_TESTING=true env variable, we see the exception whereas we don't see it in Spark 3.0. Since the exception is thrown only for "testing" mode, this would not be considered a regression?

SparkQA · 2021-01-27T06:16:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39121/

SparkQA · 2021-01-27T06:46:23Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39121/

SparkQA · 2021-01-27T10:18:35Z

Test build #134536 has finished for PR 31352 at commit 7016fa5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-01-27T19:09:43Z

@imback82 thanks for catching this issue! Since it only affects tests, I don't believe it's a real bug.

I think calling a rule in doCanonicalize is tricky, so I opened a PR to clean up this: #31368 , please take a look.

dongjoon-hyun · 2021-01-27T19:40:18Z

Thank you, @imback82 , @HyukjinKwon , and @cloud-fan .
Ya, @cloud-fan 's new PR might be a better approach.

imback82 · 2021-01-27T20:06:54Z

Thanks all! I'll close this PR and check the other one.

### What changes were proposed in this pull request? The currently SQL (temp or permanent) view resolution is done in 2 steps: 1. In `SessionCatalog`, we get the view metadata, parse the view SQL string, and wrap it with `View`. 2. At the beginning of the optimizer, we run `EliminateView`, which drops the wrapper `View`, and apply some special logic to match the view schema. Step 2 is tricky, as we need to retain the output attr expr id, while we need to add an extra `Project` to add cast and alias. This PR simplifies the view solution by building a completed plan (with cast and alias added) in `SessionCatalog`, so that we only have 1 step. ### Why are the changes needed? Code simplification. It also fixes issues like apache#31352 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? existing tests Closes apache#31368 from cloud-fan/try. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2021-01-29T06:49:11Z

Hi @imback82 , can you re-open the PR to add the tests? thanks!

SparkQA · 2021-01-30T20:26:48Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39265/

SparkQA · 2021-01-30T20:55:12Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39265/

imback82 · 2021-01-30T22:04:37Z

Hi @imback82 , can you re-open the PR to add the tests? thanks!

Updated, thanks!

SparkQA · 2021-01-31T00:16:21Z

Test build #134678 has finished for PR 31352 at commit 76772e7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master.

### What changes were proposed in this pull request? The currently SQL (temp or permanent) view resolution is done in 2 steps: 1. In `SessionCatalog`, we get the view metadata, parse the view SQL string, and wrap it with `View`. 2. At the beginning of the optimizer, we run `EliminateView`, which drops the wrapper `View`, and apply some special logic to match the view schema. Step 2 is tricky, as we need to retain the output attr expr id, while we need to add an extra `Project` to add cast and alias. This PR simplifies the view solution by building a completed plan (with cast and alias added) in `SessionCatalog`, so that we only have 1 step. ### Why are the changes needed? Code simplification. It also fixes issues like apache#31352 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? existing tests Closes apache#31368 from cloud-fan/try. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…egate's grouping expression ### What changes were proposed in this pull request? This PR is a follow-up to apache#31368 to add a test case that has a subquery with "view" in aggregate's grouping expression. The existing test tests a subquery with dataframe's temp view, so it doesn't contain a `View` node. ### Why are the changes needed? To increase the test coverage. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a new test. Closes apache#31352 from imback82/grouping_expr. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

initial commit

651a2ec

imback82 commented Jan 26, 2021

View reviewed changes

github-actions bot added the SQL label Jan 26, 2021

dongjoon-hyun reviewed Jan 26, 2021

View reviewed changes

imback82 added 3 commits January 26, 2021 20:45

formatting

de31ade

formatting

f4b2967

revert formatting

7016fa5

cloud-fan mentioned this pull request Jan 27, 2021

[SPARK-34269][SQL] Simplify SQL view resolution #31368

Closed

imback82 closed this Jan 27, 2021

imback82 deleted the grouping_expr branch January 27, 2021 20:07

imback82 changed the title ~~[SPARK-34252][SQL] Subquery with view in aggregate's grouping expression fails during the analysis check~~ [SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression Jan 30, 2021

Merge branch 'master' into grouping_expr

76772e7

imback82 reopened this Jan 30, 2021

dongjoon-hyun approved these changes Jan 31, 2021

View reviewed changes

dongjoon-hyun closed this in bec88a6 Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression #31352

[SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression #31352

imback82 commented Jan 26, 2021 •

edited

imback82 Jan 26, 2021

HyukjinKwon Jan 27, 2021

imback82 Jan 27, 2021

imback82 Jan 26, 2021

dongjoon-hyun left a comment

imback82 commented Jan 26, 2021 •

edited

dongjoon-hyun commented Jan 26, 2021

SparkQA commented Jan 27, 2021

imback82 commented Jan 27, 2021

imback82 commented Jan 27, 2021

HyukjinKwon commented Jan 27, 2021

imback82 commented Jan 27, 2021

SparkQA commented Jan 27, 2021

SparkQA commented Jan 27, 2021

SparkQA commented Jan 27, 2021

cloud-fan commented Jan 27, 2021

dongjoon-hyun commented Jan 27, 2021

imback82 commented Jan 27, 2021

cloud-fan commented Jan 29, 2021

SparkQA commented Jan 30, 2021

SparkQA commented Jan 30, 2021

imback82 commented Jan 30, 2021

SparkQA commented Jan 31, 2021

dongjoon-hyun left a comment

[SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression #31352

[SPARK-34269][SQL][TESTS][FOLLOWUP] Test a subquery with view in aggregate's grouping expression #31352

Conversation

imback82 commented Jan 26, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

imback82 Jan 26, 2021

Choose a reason for hiding this comment

HyukjinKwon Jan 27, 2021

Choose a reason for hiding this comment

imback82 Jan 27, 2021

Choose a reason for hiding this comment

imback82 Jan 26, 2021

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

imback82 commented Jan 26, 2021 • edited

dongjoon-hyun commented Jan 26, 2021

SparkQA commented Jan 27, 2021

imback82 commented Jan 27, 2021

imback82 commented Jan 27, 2021

HyukjinKwon commented Jan 27, 2021

imback82 commented Jan 27, 2021

SparkQA commented Jan 27, 2021

SparkQA commented Jan 27, 2021

SparkQA commented Jan 27, 2021

cloud-fan commented Jan 27, 2021

dongjoon-hyun commented Jan 27, 2021

imback82 commented Jan 27, 2021

cloud-fan commented Jan 29, 2021

SparkQA commented Jan 30, 2021

SparkQA commented Jan 30, 2021

imback82 commented Jan 30, 2021

SparkQA commented Jan 31, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

imback82 commented Jan 26, 2021 •

edited

imback82 commented Jan 26, 2021 •

edited