[SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions #14284

yhuai · 2016-07-20T08:57:32Z

What changes were proposed in this pull request?

This PR contains three changes.

First, this PR changes the behavior of lead/lag back to Spark 1.6's behavior, which is described as below:

lead/lag respect null input values, which means that if the offset row exists and the input value is null, the result will be null instead of the default value.
If the offset row does not exist, the default value will be used.
OffsetWindowFunction's nullable setting also considers the nullability of its input (because of the first change).

Second, this PR fixes the evaluation of lead/lag when the input expression is a literal. This fix is a result of the first change. In current master, if a literal is used as the input expression of a lead or lag function, the result will be this literal even if the offset row does not exist.

Third, this PR makes ResolveWindowFrame not fire if a window function is not resolved.

How was this patch tested?

New tests in SQLWindowFunctionSuite

…e field.

…s explained below: * When the offset row does not exits, default values will be used. * lead/lag always respect null input values.

yhuai · 2016-07-20T08:58:47Z

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

@@ -382,7 +382,7 @@ abstract class OffsetWindowFunction
 *
 * @param input expression to evaluate 'offset' rows after the current row.
 * @param offset rows to jump ahead in the partition.
- * @param default to use when the input value is null or when the offset is larger than the window.
+ * @param default to use when the offset is larger than the window.


@hvanhovell what was the reason that we changed the behavior of lead and lag on if they respect null values?

yhuai · 2016-07-20T09:07:59Z

Without a good reason and providing a way to make lead and lag respect Bulls, we should not change the behavior.

SparkQA · 2016-07-20T10:45:45Z

Test build #62593 has finished for PR 14284 at commit c2fd2d7.

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

yhuai · 2016-07-20T15:10:10Z

test this please

SparkQA · 2016-07-20T17:11:38Z

Test build #62609 has finished for PR 14284 at commit c2fd2d7.

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

SparkQA · 2016-07-20T17:46:03Z

Test build #62611 has finished for PR 14284 at commit 51e6937.

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

rxin · 2016-07-20T18:53:45Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLWindowFunctionSuite.scala

@@ -367,4 +367,50 @@ class SQLWindowFunctionSuite extends QueryTest with SQLTestUtils with TestHiveSi
        | select * from v2 order by key limit 1
      """.stripMargin), Row(0, 3))
  }
+
+  test("lead/lag should return the default value if the offset row does not exist") {


hm why is this file in hive?

can you move it in a separate pr?

oh, originally we used window functions from Hive. It should be reason that we put this file at here. Let me move it.

yhuai · 2016-07-20T20:18:18Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLWindowFunctionSuite.scala

@@ -357,14 +356,59 @@ class SQLWindowFunctionSuite extends QueryTest with SQLTestUtils with TestHiveSi
  }

  test("SPARK-7595: Window will cause resolve failed with self join") {
-    sql("SELECT * FROM src") // Force loading of src table.


For this test, I disabled the fix (https://github.com/apache/spark/pull/6114/files) and checked that it does fail the analysis because analyzer fails to resolve conflicting references in Join. So, this test is still valid after my change.

but why we remove it?

it is not in Hive. So there is no table called src.

SparkQA · 2016-07-20T22:20:57Z

Test build #62626 has finished for PR 14284 at commit faa7d89.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class WindowData(month: Int, area: String, product: Int)
- class SQLWindowFunctionSuite extends QueryTest with SharedSQLContext

jaceklaskowski · 2016-07-21T20:29:00Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLWindowFunctionSuite.scala

+      Row(2, 2))
+  }
+
+  test("lead/lag should be able to handle null input value correctly") {


I don't think "correctly" is needed here.

Can you explain?

It's best if the test case name should specify the behavior, rather than saying "correctly".

Since obviously we don't want anything to be "incorrectly"

ok I see what @jaceklaskowski meant. I thought he was questioning the behavior of lead/lag.

SparkQA · 2016-07-21T22:39:44Z

Test build #62690 has finished for PR 14284 at commit 073ac94.

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

hvanhovell · 2016-07-24T19:31:59Z

sql/core/src/main/scala/org/apache/spark/sql/execution/WindowExec.scala

@@ -625,10 +643,12 @@ private[execution] final class OffsetWindowFunctionFrame(
    if (inputIndex >= 0 && inputIndex < input.size) {


This is a more general comment, which does not necessarily apply to this line. Since we are breaking the code up into to separate code paths (with row/without row), we might as well get rid of the joined row and the logic needed to set this up (like: Seq.fill(ordinal)(NoOp))

Yea, we can improve this part in master.

Ok, lets improve this in a follow-up PR :).

yhuai · 2016-07-26T00:39:41Z

this this please

cloud-fan · 2016-07-26T01:12:46Z

should we also update the doc for OffsetWindowFunction.default?

cloud-fan · 2016-07-26T01:12:55Z

retest this please

yhuai · 2016-07-26T01:17:04Z

yea. that's a good point.

cloud-fan · 2016-07-26T01:22:56Z

LGTM

SparkQA · 2016-07-26T03:28:11Z

Test build #62854 has finished for PR 14284 at commit ff3029e.

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

yhuai · 2016-07-26T03:56:48Z

Thanks for review. I am merging this to master and branch 2.0.

…ed to lead and lag functions ## What changes were proposed in this pull request? This PR contains three changes. First, this PR changes the behavior of lead/lag back to Spark 1.6's behavior, which is described as below: 1. lead/lag respect null input values, which means that if the offset row exists and the input value is null, the result will be null instead of the default value. 2. If the offset row does not exist, the default value will be used. 3. OffsetWindowFunction's nullable setting also considers the nullability of its input (because of the first change). Second, this PR fixes the evaluation of lead/lag when the input expression is a literal. This fix is a result of the first change. In current master, if a literal is used as the input expression of a lead or lag function, the result will be this literal even if the offset row does not exist. Third, this PR makes ResolveWindowFrame not fire if a window function is not resolved. ## How was this patch tested? New tests in SQLWindowFunctionSuite Author: Yin Huai <yhuai@databricks.com> Closes #14284 from yhuai/lead-lag. (cherry picked from commit 815f3ee) Signed-off-by: Yin Huai <yhuai@databricks.com>

chengat1314 · 2017-01-04T05:51:20Z

Are we able to enable ignore null feature in Spark 2.1?
like lag(comm ignore nulls) over (order by empno) prev_comm.
thx

chengat1314 · 2017-01-04T06:53:49Z

Is possible add feature to enable ignore nulls? @yhuai
for example:
LAG (value_expr [, offset ])
[ IGNORE NULLS | RESPECT NULLS ]
OVER ( [ PARTITION BY window_partition ] ORDER BY window_ordering )

thanks
Cheng Feng

chengat1314

We should keep both IGNORE NULLS | RESPECT NULLS as feature.

hvanhovell · 2017-01-04T11:44:27Z

@chengat1314 this has never been supported, and is a new feature. There was some discussion on the JIRA a while back: https://issues.apache.org/jira/browse/SPARK-17423. Lets move the discussion there.

chengat1314 · 2017-01-05T05:56:26Z

@hvanhovell Nice, thank you very much!

yhuai added 5 commits July 19, 2016 23:56

Add regression tests

78e6901

Fix SPARK-16642

da5f36f

OffsetWindowFunction's nullable should also check its input's nullabl…

02ee191

…e field.

Change the behavior of lead/lag back to Spark 1.6's behavior, which i…

506393b

…s explained below: * When the offset row does not exits, default values will be used. * lead/lag always respect null input values.

doc change

c2fd2d7

yhuai reviewed Jul 20, 2016
View reviewed changes

yhuai changed the title ~~[SPARK-16633] [SPARK-16642] Fixes three issues related to window functions~~ [SPARK-16633] [SPARK-16642] Fixes three issues related to lead and lag functions Jul 20, 2016

Fix doc

51e6937

rxin reviewed Jul 20, 2016
View reviewed changes

Move the test file

faa7d89

yhuai reviewed Jul 20, 2016
View reviewed changes

jaceklaskowski reviewed Jul 21, 2016
View reviewed changes

test names

073ac94

hvanhovell reviewed Jul 24, 2016
View reviewed changes

yhuai changed the title ~~[SPARK-16633] [SPARK-16642] Fixes three issues related to lead and lag functions~~ [SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions Jul 25, 2016

Update doc

ff3029e

asfgit closed this in 815f3ee Jul 26, 2016

chengat1314 reviewed Jan 4, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions #14284

[SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions #14284

yhuai commented Jul 20, 2016

yhuai Jul 20, 2016

yhuai commented Jul 20, 2016 •

edited

Loading

SparkQA commented Jul 20, 2016

yhuai commented Jul 20, 2016

SparkQA commented Jul 20, 2016

SparkQA commented Jul 20, 2016

rxin Jul 20, 2016

yhuai Jul 20, 2016

yhuai Jul 20, 2016

yhuai Jul 20, 2016

cloud-fan Jul 24, 2016

yhuai Jul 25, 2016

SparkQA commented Jul 20, 2016

jaceklaskowski Jul 21, 2016

yhuai Jul 21, 2016

rxin Jul 21, 2016

yhuai Jul 21, 2016

SparkQA commented Jul 21, 2016

hvanhovell Jul 24, 2016

yhuai Jul 25, 2016

hvanhovell Jul 25, 2016

yhuai commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

yhuai commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

SparkQA commented Jul 26, 2016

yhuai commented Jul 26, 2016

chengat1314 commented Jan 4, 2017

chengat1314 commented Jan 4, 2017 •

edited

Loading

chengat1314 left a comment

hvanhovell commented Jan 4, 2017

chengat1314 commented Jan 5, 2017

		@@ -625,10 +643,12 @@ private[execution] final class OffsetWindowFunctionFrame(
		if (inputIndex >= 0 && inputIndex < input.size) {

[SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions #14284

[SPARK-16633] [SPARK-16642] [SPARK-16721] [SQL] Fixes three issues related to lead and lag functions #14284

Conversation

yhuai commented Jul 20, 2016

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

yhuai commented Jul 20, 2016 • edited Loading

SparkQA commented Jul 20, 2016

yhuai commented Jul 20, 2016

SparkQA commented Jul 20, 2016

SparkQA commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhuai commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

yhuai commented Jul 26, 2016

cloud-fan commented Jul 26, 2016

SparkQA commented Jul 26, 2016

yhuai commented Jul 26, 2016

chengat1314 commented Jan 4, 2017

chengat1314 commented Jan 4, 2017 • edited Loading

chengat1314 left a comment

Choose a reason for hiding this comment

hvanhovell commented Jan 4, 2017

chengat1314 commented Jan 5, 2017

yhuai commented Jul 20, 2016 •

edited

Loading

chengat1314 commented Jan 4, 2017 •

edited

Loading