Skip to content

[SPARK-23823][SQL] Keep origin in transformExpression #20961

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.

Sign up for GitHub

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

Jump to bottom
Closed
jiahuijiang wants to merge 4 commits into from
Closed

[SPARK-23823][SQL] Keep origin in transformExpression #20961

jiahuijiang wants to merge 4 commits into from

Conversation

@jiahuijiang
Copy link

@jiahuijiang jiahuijiang commented Apr 2, 2018

Fixes https://issues.apache.org/jira/browse/SPARK-23823

Keep origin for all the methods using transformExpression

What changes were proposed in this pull request?

Keep origin in transformExpression

How was this patch tested?

Manually tested that this fixes https://issues.apache.org/jira/browse/SPARK-23823 and columns have correct origins after Analyzer.analyze

@jiahuijiang jiahuijiang mentioned this pull request Apr 2, 2018
Closed
@gatorsmile
Copy link
Member

gatorsmile commented Apr 2, 2018

ok to test

@SparkQA
Copy link

SparkQA commented Apr 3, 2018

Test build #88831 has finished for PR 20961 at commit 5e83805.

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

gatorsmile
gatorsmile reviewed Apr 4, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala Outdated

@inline def transformExpression(e: Expression): Expression = {
val newE = f(e)
val newE = CurrentOrigin.withOrigin(e.origin) { f(e) }
Copy link
Member

@gatorsmile gatorsmile Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: style issue:

      val newE = CurrentOrigin.withOrigin(e.origin) {
        f(e)
      }

@gatorsmile
Copy link
Member

gatorsmile commented Apr 4, 2018

This sounds right to me. cc @hvanhovell @cloud-fan

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 4, 2018

LGTM, too

@jiahuijiang
Copy link
Author

jiahuijiang commented Apr 4, 2018

@gatorsmile fixed :D

@hvanhovell
Copy link
Contributor

hvanhovell commented Apr 4, 2018

Can you add a test? Just show that origin is still the same after you transform the node.

@SparkQA
Copy link

SparkQA commented Apr 4, 2018

Test build #88890 has finished for PR 20961 at commit 8396dbb.

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

@jiahuijiang
Copy link
Author

jiahuijiang commented Apr 4, 2018

@hvanhovell added a test, before this change the origin would be undefined

@SparkQA
Copy link

SparkQA commented Apr 5, 2018

Test build #88909 has finished for PR 20961 at commit 52e8875.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class QueryPlanSuite extends SparkFunSuite

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 5, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 5, 2018

Test build #88945 has finished for PR 20961 at commit 52e8875.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class QueryPlanSuite extends SparkFunSuite

@kiszk
Copy link
Member

kiszk commented Apr 5, 2018

retest this please

kiszk
kiszk reviewed Apr 5, 2018
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/QueryPlanSuite.scala Outdated

class QueryPlanSuite extends SparkFunSuite {

test("origin remains the same after mapExpressions") {
Copy link
Member

@kiszk kiszk Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is it better to add JIRA number (i.e. SPARK-PARK-23823: ...)

@jiahuijiang
Copy link
Author

jiahuijiang commented Apr 5, 2018

@kiszk Added the ticket number! Do we need to retrigger the test build on that new commit?

@SparkQA
Copy link

SparkQA commented Apr 5, 2018

Test build #88954 has finished for PR 20961 at commit a1c029c.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 5, 2018

Test build #88952 has finished for PR 20961 at commit 52e8875.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class QueryPlanSuite extends SparkFunSuite

@jiahuijiang
Copy link
Author

jiahuijiang commented Apr 5, 2018

Don't know why it failed so many times... but all these tests pass on my local machine and the commit before adding the test...

@gatorsmile
Copy link
Member

gatorsmile commented Apr 5, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 6, 2018

Test build #88957 has finished for PR 20961 at commit a1c029c.

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

@gatorsmile
Copy link
Member

gatorsmile commented Apr 6, 2018

Thanks! Merged to master/2.3

@asfgit asfgit closed this in d65e531 Apr 6, 2018
asfgit pushed a commit that referenced this pull request Apr 6, 2018
@gatorsmile
[SPARK-23823][SQL] Keep origin in transformExpression
f93667f 
Fixes https://issues.apache.org/jira/browse/SPARK-23823

Keep origin for all the methods using transformExpression

## What changes were proposed in this pull request?

Keep origin in transformExpression

## How was this patch tested?

Manually tested that this fixes https://issues.apache.org/jira/browse/SPARK-23823 and columns have correct origins after Analyzer.analyze

Author: JiahuiJiang <[email protected]>
Author: Jiahui Jiang <[email protected]>

Closes #20961 from JiahuiJiang/jj/keep-origin.

(cherry picked from commit d65e531)
Signed-off-by: gatorsmile <[email protected]>
robert3005 pushed a commit to palantir/spark that referenced this pull request Apr 7, 2018
[SPARK-23823][SQL] Keep origin in transformExpression
9ff54b1 
Fixes https://issues.apache.org/jira/browse/SPARK-23823

Keep origin for all the methods using transformExpression

## What changes were proposed in this pull request?

Keep origin in transformExpression

## How was this patch tested?

Manually tested that this fixes https://issues.apache.org/jira/browse/SPARK-23823 and columns have correct origins after Analyzer.analyze

Author: JiahuiJiang <[email protected]>
Author: Jiahui Jiang <[email protected]>

Closes apache#20961 from JiahuiJiang/jj/keep-origin.
peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
[SPARK-23823][SQL] Keep origin in transformExpression
c854344 
Fixes https://issues.apache.org/jira/browse/SPARK-23823

Keep origin for all the methods using transformExpression

Keep origin in transformExpression

Manually tested that this fixes https://issues.apache.org/jira/browse/SPARK-23823 and columns have correct origins after Analyzer.analyze

Author: JiahuiJiang <[email protected]>
Author: Jiahui Jiang <[email protected]>

Closes apache#20961 from JiahuiJiang/jj/keep-origin.

(cherry picked from commit d65e531)
Signed-off-by: gatorsmile <[email protected]>

Change-Id: I6cd7636a051144f5eb2d079f141ae69616dcad7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiszk kiszk kiszk left review comments

@gatorsmile gatorsmile gatorsmile left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jiahuijiang @gatorsmile @SparkQA @cloud-fan @hvanhovell @kiszk