[SPARK-9740] [SPARK-9592] [SPARK-9210] [SQL] Change the default behavior of First/Last to RESPECT NULLS. #8113

yhuai · 2015-08-11T22:29:21Z

I am changing the default behavior of First/Last to respect null values (the SQL standard default behavior).

https://issues.apache.org/jira/browse/SPARK-9740

yhuai · 2015-08-11T22:29:57Z

@hvanhovell I am making the default in this PR.

SparkQA · 2015-08-12T01:08:04Z

Test build #40531 has finished for PR 8113 at commit 6ee4a04.

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

hvanhovell · 2015-08-12T01:10:50Z

One final question, shouldn't we introduce a skipNulls parameter? Or do you want to address this in a follow-up PR?

hvanhovell · 2015-08-12T01:15:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/functions.scala

We could for now simplify this to Literal.create(true, BooleanType).

yhuai · 2015-08-12T01:16:43Z

yeah. I think that only requires a small change. Let me add it.

hvanhovell · 2015-08-12T01:17:44Z

Besides the valueSet update/merge expressions LGTM.

yhuai · 2015-08-12T04:20:20Z

@hvanhovell Alright. I added the ignoreNulls flag.

yhuai · 2015-08-12T04:31:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregates.scala

@ggupta81 This is the fix of SPARK-9592 for our master and 1.5 branch.

ggupta81 · 2015-08-12T04:49:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregates.scala

@yhuai Both if and else branches are executing the same code.

oops. Will fix it.

ggupta81 · 2015-08-12T05:12:23Z

Should i close my original pull request now?
#7928

On Wed, Aug 12, 2015 at 10:38 AM, UCB AMPLab notifications@github.com
wrote:

Merged build started.

—
Reply to this email directly or view it on GitHub
#8113 (comment).

_Gaurav Gupta_Engineering Manager @ Adobe
9971666780

yhuai · 2015-08-12T05:16:00Z

@ggupta81 Yeah. We can close #7928. Can you open a pr to fix branch 1.4, or reopen #7233 and update that? Thanks!

SparkQA · 2015-08-12T06:32:48Z

Test build #40589 has finished for PR 8113 at commit 15c9e31.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class First(child: Expression, ignoreNulls: Boolean) extends AlgebraicAggregate
- case class Last(child: Expression, ignoreNulls: Boolean) extends AlgebraicAggregate
- case class First(
- case class FirstFunction(
- case class Last(
- case class LastFunction(

SparkQA · 2015-08-12T09:46:22Z

Test build #1488 has finished for PR 8113 at commit 2d93d89.

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

hvanhovell · 2015-08-12T15:24:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/functions.scala

Nit: Or(valueSet, Not(IsNull(child))) is a bit shorter. It is a matter of preference though...

hvanhovell · 2015-08-12T15:31:24Z

One more small thing. We should probably also add the ignoreNulls option to the first and last dataframe functions.

hvanhovell · 2015-08-12T15:55:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/functions.scala

Nit: Or(valueSet.left. valueSet.right) is shorter.

yhuai · 2015-08-13T00:54:05Z

@hvanhovell Since we already pass the feature freeze deadline, I will not add new interfaces to DataFrame API (this PR is mainly about fixing the default behavior). If users request adding DF functions that expose ignoreNulls flag, we can do it in 1.6.

SparkQA · 2015-08-13T02:49:06Z

Test build #40705 has finished for PR 8113 at commit ad0ac67.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class First(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class Last(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class First(
- case class FirstFunction(
- case class Last(
- case class LastFunction(

SparkQA · 2015-08-13T05:21:57Z

Test build #40724 has finished for PR 8113 at commit f828bdf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class First(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class Last(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class First(
- case class FirstFunction(
- case class Last(
- case class LastFunction(

SparkQA · 2015-08-13T06:23:21Z

Test build #1542 has finished for PR 8113 at commit f828bdf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * Set thresholds in multiclass (or binary) classification to adjust the probability of
- case class First(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class Last(child: Expression, ignoreNullsExpr: Expression) extends AlgebraicAggregate
- case class First(
- case class FirstFunction(
- case class Last(
- case class LastFunction(

SparkQA · 2015-08-13T18:50:41Z

Test build #1551 timed out for PR 8113 at commit f828bdf after a configured wait of 175m.

…pression1. https://issues.apache.org/jira/browse/SPARK-9592 #8113 has the fundamental fix. But, if we want to minimize the number of changed lines, we can go with this one. Then, in 1.6, we merge #8113. Author: Yin Huai <yhuai@databricks.com> Closes #8172 from yhuai/lastFix and squashes the following commits: b28c42a [Yin Huai] Regression test. af87086 [Yin Huai] Fix last. (cherry picked from commit 772e7c1) Signed-off-by: Michael Armbrust <michael@databricks.com>

…pression1. https://issues.apache.org/jira/browse/SPARK-9592 #8113 has the fundamental fix. But, if we want to minimize the number of changed lines, we can go with this one. Then, in 1.6, we merge #8113. Author: Yin Huai <yhuai@databricks.com> Closes #8172 from yhuai/lastFix and squashes the following commits: b28c42a [Yin Huai] Regression test. af87086 [Yin Huai] Fix last.

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/functions.scala sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregates.scala

yhuai · 2015-10-19T23:11:52Z

test this please

SparkQA · 2015-10-20T02:31:15Z

Test build #43948 has finished for PR 8113 at commit 91c174f.

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

marmbrus · 2015-10-21T20:42:54Z

Thanks! Merging to master.

hvanhovell reviewed Aug 12, 2015
View reviewed changes

yhuai reviewed Aug 12, 2015
View reviewed changes

yhuai changed the title ~~[SPARK-9740] [SQL] Change the default behavior of First/Last to RESPECT NULLS.~~ [SPARK-9740] [SPARK-9592] [SQL] Change the default behavior of First/Last to RESPECT NULLS. Aug 12, 2015

ggupta81 reviewed Aug 12, 2015
View reviewed changes

ggupta81 mentioned this pull request Aug 12, 2015

[SPARK-9592] [SQL] Fixed First and Last aggregates to calculate on GroupedData partition instead of entire dataFrame #7928

Closed

hvanhovell reviewed Aug 12, 2015
View reviewed changes

yhuai mentioned this pull request Aug 12, 2015

SPARK-9210 corrects aggregate function name in exception message #7557

Closed

yhuai added 6 commits August 12, 2015 16:53

Change the default behavior of First/Last to RESPECT NULLS.

9662a2f

Add ignoreNulls flag.

a043171

Fix LastFunction's update.

cc40a90

Remove unnecessary change.

ad0e120

Update

90a72b7

Update test.

ad0ac67

Fix test.

f828bdf

yhuai changed the title ~~[SPARK-9740] [SPARK-9592] [SQL] Change the default behavior of First/Last to RESPECT NULLS.~~ [SPARK-9740] [SPARK-9592] [SPARK-9210] [SQL] Change the default behavior of First/Last to RESPECT NULLS. Aug 13, 2015

yhuai mentioned this pull request Aug 13, 2015

[SPARK-9592] [SQL] Fix Last function implemented based on AggregateExpression1. #8172

Closed

Merge remote-tracking branch 'upstream/master' into firstLast

91c174f

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/functions.scala sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregates.scala

asfgit closed this in 3afe448 Oct 21, 2015

[SPARK-9740] [SPARK-9592] [SPARK-9210] [SQL] Change the default behavior of First/Last to RESPECT NULLS. #8113

[SPARK-9740] [SPARK-9592] [SPARK-9210] [SQL] Change the default behavior of First/Last to RESPECT NULLS. #8113

Uh oh!

Conversation

yhuai commented Aug 11, 2015

Uh oh!

yhuai commented Aug 11, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

hvanhovell commented Aug 12, 2015

Uh oh!

hvanhovell Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

yhuai commented Aug 12, 2015

Uh oh!

hvanhovell commented Aug 12, 2015

Uh oh!

yhuai commented Aug 12, 2015

Uh oh!

yhuai Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

ggupta81 Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

yhuai Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

ggupta81 commented Aug 12, 2015

Uh oh!

yhuai commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

hvanhovell Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Aug 12, 2015

Uh oh!

hvanhovell Aug 12, 2015

Choose a reason for hiding this comment

Uh oh!

yhuai commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

yhuai commented Oct 19, 2015

Uh oh!

SparkQA commented Oct 20, 2015

Uh oh!

marmbrus commented Oct 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants