[SPARK-33036][SQL] Refactor RewriteCorrelatedScalarSubquery code to replace exprIds in a bottom-up manner by maropu · Pull Request #29913 · apache/spark

maropu · 2020-09-30T13:05:39Z

What changes were proposed in this pull request?

This PR intends to refactor code in RewriteCorrelatedScalarSubquery for replacing ExprIds in a bottom-up manner instead of doing in a top-down one.

This PR comes from the talk with @cloud-fan in #29585 (comment).

Why are the changes needed?

To improve code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

SparkQA · 2020-09-30T13:51:17Z

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

SparkQA · 2020-09-30T14:15:18Z

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

SparkQA · 2020-09-30T18:59:44Z

Test build #129274 has finished for PR 29913 at commit 46a227e.

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

maropu · 2020-09-30T23:08:33Z

cc: @cloud-fan

dongjoon-hyun · 2020-10-02T00:20:41Z

Hi, @maropu .
Could you check the original PR's performance effect? Also, I'm wondering if this PR can recover it.

[SPARK-32741][SQL] Check if the same ExprId refers to the unique attribute in logical plans #29585 (comment)

cloud-fan · 2020-10-06T13:03:35Z

retest this please

cloud-fan · 2020-10-06T13:08:45Z

+ val newExprs = exprs.map { _.transform {
+ case a: AttributeReference if attrMap.contains(a) =>
+ val exprId = attrMap.getOrElse(a, a).exprId
+ a.withExprId(exprId)


shall we simply use attrMap(a) or only use the exprId?

SparkQA · 2020-10-06T13:17:21Z

Test build #129445 has finished for PR 29913 at commit 46a227e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-06T14:06:05Z

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

SparkQA · 2020-10-06T14:27:06Z

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

SparkQA · 2020-10-07T00:46:15Z

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

SparkQA · 2020-10-07T01:07:54Z

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

cloud-fan · 2020-10-07T04:53:48Z

 }
 }
 }
+ (newChild, AttributeMap(subqueryAttrMapping))


AttributeMap(subqueryAttrMapping.toSeq) to pass scala 2.13 compilation

Ah, I missed that. Updated.

SparkQA · 2020-10-07T05:39:44Z

Test build #129480 has finished for PR 29913 at commit 405be2d.

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

SparkQA · 2020-10-07T05:55:07Z

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

SparkQA · 2020-10-07T06:11:08Z

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

SparkQA · 2020-10-07T07:05:02Z

Test build #129490 has finished for PR 29913 at commit 47ea616.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-10-07T07:28:56Z

+ attrMap: AttributeMap[Attribute]): Seq[E] = {
+ if (attrMap.nonEmpty) {
+ val newExprs = exprs.map { _.transform {
+ case a: AttributeReference if attrMap.contains(a) => attrMap(a)


not a big deal: it's more efficient to write case a: AttributeReference => attrMap.getOrElse(a, a)

SparkQA · 2020-10-07T08:41:55Z

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

SparkQA · 2020-10-07T09:00:39Z

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

maropu · 2020-10-07T11:17:37Z

GA passed, so merged to master. Thanks, @cloud-fan

SparkQA · 2020-10-07T12:37:43Z

Test build #129496 has finished for PR 29913 at commit cbd0c5c.

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

Fix
46a227e

probot-autolabelerBot added the SQL label Sep 30, 2020

cloud-fan reviewed Oct 6, 2020
View reviewed changes

Fix
405be2d

cloud-fan reviewed Oct 7, 2020
View reviewed changes

Fix
47ea616

cloud-fan reviewed Oct 7, 2020
View reviewed changes

cloud-fan approved these changes Oct 7, 2020
View reviewed changes

Fix
cbd0c5c

maropu closed this in 94d648dOct 7, 2020

Uh oh!

Conversation

maropu commented Sep 30, 2020

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?

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

SparkQA commented Sep 30, 2020

Uh oh!

maropu commented Sep 30, 2020

Uh oh!

dongjoon-hyun commented Oct 2, 2020

Uh oh!

cloud-fan commented Oct 6, 2020

Uh oh!

cloud-fanOct 6, 2020

Choose a reason for hiding this comment

Uh oh!

maropuOct 6, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 6, 2020

Uh oh!

SparkQA commented Oct 6, 2020

Uh oh!

SparkQA commented Oct 6, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

cloud-fanOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

maropuOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

cloud-fanOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

maropu commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants