Skip to content
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

[SPARK-16648][SQL] Make ignoreNullsExpr a child expression of First and Last #14295

Closed
wants to merge 3 commits into from
Closed

[SPARK-16648][SQL] Make ignoreNullsExpr a child expression of First and Last #14295

wants to merge 3 commits into from

Conversation

liancheng
Copy link
Contributor

@liancheng liancheng commented Jul 21, 2016 •
edited
Loading

What changes were proposed in this pull request?

Default TreeNode.withNewChildren implementation doesn't work for Last and when both constructor arguments are the same, e.g.:

LAST_VALUE(FALSE) -- The 2nd argument defaults to FALSE
LAST_VALUE(FALSE, FALSE)
LAST_VALUE(TRUE, TRUE)

This is because although Last is a unary expression, both of its constructor arguments, child and ignoreNullsExpr, are Expressions. When they have the same value, TreeNode.withNewChildren treats both of them as child nodes by mistake. First is also affected by this issue in exactly the same way.

This PR fixes this issue by making ignoreNullsExpr a child expression of First and Last.

How was this patch tested?

New test case added in WindowQuerySuite.

@SparkQA
Copy link

SparkQA commented Jul 21, 2016

Test build #62654 has finished for PR 14295 at commit efbed91.

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

jaceklaskowski
jaceklaskowski reviewed Jul 22, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: Expression) extends Declarat

override def children: Seq[Expression] = child :: Nil

// SPARK-16648: Default `TreeNode.withNewChildren` implementation doesn't work for `Last` when
Copy link
Contributor

Choose a reason for hiding this comment

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

Use [[ to reference code symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a ScalaDoc comment, so [[...]] is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make it so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's probably unnecessary, since expressions are not part of the public API and won't appear in the generated ScalaDoc anyway...

@yhuai
Copy link
Contributor

yhuai commented Jul 22, 2016

@liancheng Can you also change First? I think that one is also broken for this case.

@liancheng
Copy link
Contributor Author

Oh, that's a good point, should have realized both of them are affected. Updated. Thanks!

@SparkQA
Copy link

SparkQA commented Jul 23, 2016

Test build #62750 has finished for PR 14295 at commit dd73681.

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

cloud-fan
cloud-fan reviewed Jul 24, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/First.scala
@@ -45,6 +45,17 @@ case class First(child: Expression, ignoreNullsExpr: Expression) extends Declara

override def children: Seq[Expression] = child :: Nil
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make ignoreNullsExpr also a child?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, this looks like a cleaner fix. Let me try whether it breaks anything else. Thanks!

@liancheng
Copy link
Contributor Author

Tried to make ignoreNullsExpr also a child node of First and Last in the last commit as @cloud-fan suggested. Hopefully this is a cleaner fix.

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62799 has finished for PR 14295 at commit 0152075.

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

@cloud-fan
Copy link
Contributor

LGTM

@liancheng liancheng changed the title [SPARK-16648][SQL] Overrides TreeNode.withNewChildren in Last [SPARK-16648][SQL] Make ignoreNullsExpr a child expression of First and Last Jul 25, 2016
@asfgit asfgit closed this in 68b4020 Jul 25, 2016
asfgit pushed a commit that referenced this pull request Jul 25, 2016
@liancheng @cloud-fan
[SPARK-16648][SQL] Make ignoreNullsExpr a child expression of First a…
fcbb7f6 
…nd Last

## What changes were proposed in this pull request?

Default `TreeNode.withNewChildren` implementation doesn't work for `Last` and when both constructor arguments are the same, e.g.:

```sql
LAST_VALUE(FALSE) -- The 2nd argument defaults to FALSE
LAST_VALUE(FALSE, FALSE)
LAST_VALUE(TRUE, TRUE)
```

This is because although `Last` is a unary expression, both of its constructor arguments, `child` and `ignoreNullsExpr`, are `Expression`s. When they have the same value, `TreeNode.withNewChildren` treats both of them as child nodes by mistake. `First` is also affected by this issue in exactly the same way.

This PR fixes this issue by making `ignoreNullsExpr` a child expression of `First` and `Last`.

## How was this patch tested?

New test case added in `WindowQuerySuite`.

Author: Cheng Lian <lian@databricks.com>

Closes #14295 from liancheng/spark-16648-last-value.

(cherry picked from commit 68b4020)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan
Copy link
Contributor

thanks, merging to master/2.0!

@liancheng liancheng deleted the spark-16648-last-value branch August 4, 2016 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liancheng @SparkQA @yhuai @cloud-fan @jaceklaskowski