[SPARK-36323][SQL] Support ANSI interval literals for TimeWindow by sarutak · Pull Request #33551 · apache/spark

sarutak · 2021-07-28T04:41:12Z

What changes were proposed in this pull request?

This PR proposes to support ANSI interval literals for TimeWindow.

Why are the changes needed?

Watermark also supports ANSI interval literals so it's great to support for TimeWindow.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New test.

SparkQA · 2021-07-28T05:34:55Z

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

SparkQA · 2021-07-28T06:12:54Z

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

…interval

SparkQA · 2021-07-28T09:31:34Z

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

SparkQA · 2021-07-28T10:07:02Z

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

SparkQA · 2021-07-28T11:06:24Z

Test build #141751 has finished for PR 33551 at commit 85eb021.

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

MaxGekk · 2021-07-28T13:26:27Z

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

+      case NonFatal(e) =>
+        throw QueryCompilationErrors.cannotParseTimeDelayError(interval, e)
    }
    cal.days * MICROS_PER_DAY + cal.microseconds


Not related to the PR but:

* and + can overflow. I think we should use exact arithmetic ops here.

one day in CalendarInterval can be 23, 24, or 25 hours, see

spark/common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

Line 42 in 0494dc9

* and one day may be equal to 23, 24 or 25 hours (daylight saving).

O.K, let's fix this in this PR.

For 1, I've fixed in this PR.
For 2, it's about intervals, not timestamps so we don't need to consider daylight saving here right?

For 2, it's about intervals, not timestamps so we don't need to consider daylight saving here right?

ok. Let's keep the assumption of 24 hours per day here. Not sure it will work fine during daylight saving time but it seems we don't have enough test coverage for that at the moment.

Maybe, open an JIRA to test the case?

Hmm, I'll check it out and open a JIRA.

MaxGekk · 2021-07-28T13:30:40Z

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

-      throw new IllegalArgumentException(
-        s"Intervals greater than a month is not supported ($interval).")
+    val ymIntervalErrMsg = s"Intervals greater than a month is not supported ($interval)."
+    val cal = try {


This is similar to

spark/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala

Lines 744 to 758 in 07fa38e

val parsedDelay = try {

if (delayThreshold.toLowerCase(Locale.ROOT).trim.startsWith("interval")) {

CatalystSqlParser.parseExpression(delayThreshold) match {

case Literal(months: Int, _: YearMonthIntervalType) =>

new CalendarInterval(months, 0, 0)

case Literal(micros: Long, _: DayTimeIntervalType) =>

new CalendarInterval(0, 0, micros)

}

} else {

IntervalUtils.stringToInterval(UTF8String.fromString(delayThreshold))

}

} catch {

case NonFatal(e) =>

throw QueryCompilationErrors.cannotParseTimeDelayError(delayThreshold, e)

}

. Could you put the code to a common helper function, and re-use it here and in Dataset

At first I tried to put a common helper function but they are slightly different so I didn't.
But it's O.K, to try to factor out the similar code.

MaxGekk · 2021-07-28T13:32:15Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeWindowSuite.scala

 import org.apache.spark.sql.types.{LongType, StructField, StructType, TimestampNTZType, TimestampType}

 class TimeWindowSuite extends SparkFunSuite with ExpressionEvalHelper with PrivateMethodTester {
-


Let's avoid unnecessary changes. This can cause conflicts in down stream.

Oops. Thanks.

MaxGekk · 2021-07-28T13:40:35Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeWindowSuite.scala

      Seq(StructField("start", TimestampNTZType), StructField("end", TimestampNTZType))))
  }
+
+  test("SPARK-36323: Support ANSI interval literals for TimeWindow") {


Could you test when spark.sql.legacy.interval.enabled is true?

SparkQA · 2021-07-28T13:47:31Z

Test build #141762 has finished for PR 33551 at commit 2db8f0b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait AlterTableColumnCommand extends UnaryCommand
case class AlterTableAddColumns(
case class AlterTableReplaceColumns(

SparkQA · 2021-07-28T19:19:48Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46295/

SparkQA · 2021-07-28T23:20:59Z

Test build #141783 has finished for PR 33551 at commit 5b80f21.

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

MaxGekk

+1, LGTM. Merging to master/3.2.
Thank you, @sarutak .

### What changes were proposed in this pull request? This PR proposes to support ANSI interval literals for `TimeWindow`. ### Why are the changes needed? Watermark also supports ANSI interval literals so it's great to support for `TimeWindow`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New test. Closes #33551 from sarutak/window-interval. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit db18866) Signed-off-by: Max Gekk <max.gekk@gmail.com>

…legacy.interval.enabled is true ### What changes were proposed in this pull request? This PR adds test considering the case `spark.sql.legacy.interval.enabled` is `true` for SPARK-35815. ### Why are the changes needed? SPARK-35815 (#33456) changes `Dataset.withWatermark` to accept ANSI interval literals as `delayThreshold` but I noticed the change didn't work with `spark.sql.legacy.interval.enabled=true`. We can't detect this issue because there is no test which considers the legacy interval type at that time. In SPARK-36323 (#33551), this issue was resolved but it's better to add test. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New test. Closes #33606 from sarutak/test-watermark-with-legacy-interval. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…legacy.interval.enabled is true ### What changes were proposed in this pull request? This PR adds test considering the case `spark.sql.legacy.interval.enabled` is `true` for SPARK-35815. ### Why are the changes needed? SPARK-35815 (#33456) changes `Dataset.withWatermark` to accept ANSI interval literals as `delayThreshold` but I noticed the change didn't work with `spark.sql.legacy.interval.enabled=true`. We can't detect this issue because there is no test which considers the legacy interval type at that time. In SPARK-36323 (#33551), this issue was resolved but it's better to add test. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New test. Closes #33606 from sarutak/test-watermark-with-legacy-interval. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 92cdb17) Signed-off-by: Max Gekk <max.gekk@gmail.com>

Support ANSI interval literals for TimeWindow.

85eb021

github-actions bot added the SQL label Jul 28, 2021

Merge branch 'master' of https://github.com/apache/spark into window-…

2db8f0b

…interval

MaxGekk reviewed Jul 28, 2021

View reviewed changes

Factor out common code.

5b80f21

MaxGekk approved these changes Jul 29, 2021

View reviewed changes

MaxGekk closed this in db18866 Jul 29, 2021

sarutak mentioned this pull request Aug 2, 2021

[SPARK-35815][SQL][FOLLOWUP] Add test considering the case spark.sql.legacy.interval.enabled is true #33606

Closed

	val parsedDelay = try {
	if (delayThreshold.toLowerCase(Locale.ROOT).trim.startsWith("interval")) {
	CatalystSqlParser.parseExpression(delayThreshold) match {
	case Literal(months: Int, _: YearMonthIntervalType) =>
	new CalendarInterval(months, 0, 0)
	case Literal(micros: Long, _: DayTimeIntervalType) =>
	new CalendarInterval(0, 0, micros)
	}
	} else {
	IntervalUtils.stringToInterval(UTF8String.fromString(delayThreshold))
	}
	} catch {
	case NonFatal(e) =>
	throw QueryCompilationErrors.cannotParseTimeDelayError(delayThreshold, e)
	}

		import org.apache.spark.sql.types.{LongType, StructField, StructType, TimestampNTZType, TimestampType}

		class TimeWindowSuite extends SparkFunSuite with ExpressionEvalHelper with PrivateMethodTester {

Conversation

sarutak commented Jul 28, 2021

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 Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

SparkQA commented Jul 28, 2021

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants