[SPARK-29679][SQL] Make interval type comparable and orderable #26337

yaooqinn · 2019-10-31T06:15:32Z

What changes were proposed in this pull request?

interval type support >, >=, <, <=, =, <=>, order by, min,max..

Why are the changes needed?

Part of SPARK-27764 Feature Parity between PostgreSQL and Spark

Does this PR introduce any user-facing change?

yes, we now support compare intervals

How was this patch tested?

add ut

SparkQA · 2019-10-31T07:05:02Z

Test build #112997 has finished for PR 26337 at commit 74ce207.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-10-31T07:09:47Z

retest this please

MaxGekk

Don't think CalendarIntervalType values could be comparable while the microseconds field is not limited.

MaxGekk · 2019-10-31T08:12:16Z

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

+        return -1;
+      }
+    } else {
+      return mc;


How about interval 1 month 120 days and interval 2 month?

you're right, will fix this, thanks.

SparkQA · 2019-10-31T08:46:36Z

Test build #113008 has finished for PR 26337 at commit 39d0665.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-10-31T09:16:57Z

retest this please

SparkQA · 2019-10-31T09:29:14Z

Test build #113001 has finished for PR 26337 at commit 74ce207.

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

yaooqinn · 2019-10-31T09:39:05Z

retest this please

SparkQA · 2019-10-31T09:44:41Z

Test build #113014 has finished for PR 26337 at commit 39d0665.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-31T11:21:05Z

Test build #113012 has finished for PR 26337 at commit 39d0665.

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

SparkQA · 2019-10-31T13:22:45Z

Test build #113024 has finished for PR 26337 at commit 4bba921.

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

yaooqinn · 2019-10-31T13:30:50Z

retest this please

SparkQA · 2019-10-31T16:09:08Z

Test build #113029 has finished for PR 26337 at commit 4bba921.

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

yaooqinn · 2019-11-01T03:27:24Z

retest this please

SparkQA · 2019-11-01T03:34:18Z

Test build #113065 has finished for PR 26337 at commit 4bba921.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-01T19:14:51Z

Test build #113103 has finished for PR 26337 at commit 9c842ec.

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

yaooqinn · 2019-11-04T11:11:18Z

ping @cloud-fan @maropu @HyukjinKwon @dongjoon-hyun, thanks in advance.

yaooqinn · 2019-11-05T02:43:51Z

also cc @maropu

cloud-fan · 2019-11-05T02:51:04Z

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

  public static final long MICROS_PER_HOUR = MICROS_PER_MINUTE * 60;
  public static final long MICROS_PER_DAY = MICROS_PER_HOUR * 24;
  public static final long MICROS_PER_WEEK = MICROS_PER_DAY * 7;
+  public static final long MICROS_PER_MONTH = MICROS_PER_DAY * 30;


where do we use it?

I leave it here by mistake, will delete it

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

cloud-fan · 2019-11-05T02:56:19Z

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


+  @Override
+  public int compare(CalendarInterval that) {
+    long thisAdjustDays = this.microseconds / MICROS_PER_DAY + this.days + this.months * 30;


@MaxGekk IIRC we have a different definition of days-per-month in Spark?

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

Line 38 in a4382f7

final val DAYS_PER_MONTH: Byte = 30

I just find one

31 days per months is used at the moment, see

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

Lines 333 to 343 in a4382f7

* @param daysPerMonth The number of days per one month. The default value is 31 days

* per month. This value was taken as the default because it is used

* in Structured Streaming for watermark calculations. Having 31 days

* per month, we can guarantee that events are not dropped before

* the end of any month (February with 29 days or January with 31 days).

* @return Duration in the specified time units

*/

def getDuration(

interval: CalendarInterval,

targetUnit: TimeUnit,

daysPerMonth: Int = 31): Long = {

30 is for PostgreSQL compatibility in

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

Lines 92 to 98 in a4382f7

def getEpoch(interval: CalendarInterval): Decimal = {

var result = interval.microseconds

result += DateTimeUtils.MICROS_PER_DAY * interval.days

result += MICROS_PER_YEAR * (interval.months / MONTHS_PER_YEAR)

result += MICROS_PER_MONTH * (interval.months % MONTHS_PER_YEAR)

Decimal(result, 18, 6)

}

ok let's use 30 here

How about to extract the code:

var result = interval.microseconds result += DateTimeUtils.MICROS_PER_DAY * interval.days result += MICROS_PER_YEAR * (interval.months / MONTHS_PER_YEAR) result += MICROS_PER_MONTH * (interval.months % MONTHS_PER_YEAR)

and reuse it in the comparison? At least, we will have consistent behavior.

It not accurate enough for MICROS_PER_YEAR

Your code is not accurate too. 30 * 12 = 360 is slightly far away from average days per year - 365.2425, see #25998

To be accurate, we need to convert everything to microseconds and compare them, right? i.e. interval 30 days is smaller than internal 1 month.

postgres=# select interval '1 year' = interval '360 days'; ?column? ---------- t (1 row) postgres=# select interval '1 mon' = interval '30 days'; ?column? ---------- t (1 row) postgres=# select interval '1 mon' = interval '31 days'; ?column? ---------- f (1 row) postgres=# select interval '1 year' = interval '365 days'; ?column? ---------- f (1 row)

If we are more likely to keep compatibility with pg, so a year is 360 days not 365 days, a month is 30 days

In Postgres, interval comparison will treat year as 12 months if comparing with month, 360 days if comparing day and 8640 hours if comparing with hours. This is different from date add/subtract, where '1 year' is based on what kind of year it is added to.

This is same as interval division

postgres=# select interval '1 year' / 360; ?column? ----------- 1 0:00:00 (1 row) postgres=# select interval '1 year' / 365; ?column? --------------- 23:40:16.4064 (1 row)

SparkQA · 2019-11-05T04:55:16Z

Test build #113234 has finished for PR 26337 at commit 34c794a.

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

maropu · 2019-11-05T05:57:22Z

(I looked over this pr and I have no comment except for the current discussing ones)

yaooqinn · 2019-11-05T06:01:44Z

@maropu thank you very much for your review.

yaooqinn · 2019-11-07T16:04:02Z

Oracle, mssql, presto etc. have two interval types which are the interval year to month and interval day to second, the binary comparison operators can not be applied to these two different types. I have it tested with presto with the following result as you can see. even with expilict casting.

presto> select interval '30' day = interval '1' month;
Query 20191107_145330_00007_f239d failed: line 1:26: '=' cannot be applied to interval day to second, interval year to month
select interval '30' day = interval '1' month

presto> select interval '30' day < interval '1' month;
Query 20191107_150903_00008_f239d failed: line 1:26: '<' cannot be applied to interval day to second, interval year to month
select interval '30' day < interval '1' month

presto>
presto> select interval '30' day < cast(interval '1' month as interval day to second);
Query 20191107_153514_00009_f239d failed: line 1:28: Cannot cast interval year to month to interval day to second
select interval '30' day < cast(interval '1' month as interval day to second)

As we are more likely to define interval types and parse them as consistent as PostgreSQL’s approach not other dbs.
So option 1: when we do a binary comparison between two intervals where we need to adjust year-month to day-second part, we follow Postgres as #26337 (comment) and #26337 (comment) to keep feature parity with postgres. This is also current pr's approach. It's kind of "accurate" when only intervals themselves participate in calculating. We may need to a doc to notice users for such behavior

Option 1: Use a year with a average value of 365.25 days and a month of 30 days to adjust.

@cloud-fan @MaxGekk @maropu I hope we can reach an agreement here soon, thanks for your time.

cloud-fan · 2019-11-07T16:13:56Z

so it's mostly about years. 1) 1 year = 12 month = 360 days, or 2) 1 year = 362.25 days

I think option 1 is more consistent. What does pgsql do?

yaooqinn · 2019-11-07T16:20:47Z

Postgres use option 1 that a year is 360 days and a month is 30 day in interval binary comparison. As the test case I listed here #26337 (comment)

cloud-fan · 2019-11-07T16:28:17Z

let's go option 1 then

SparkQA · 2019-11-07T17:50:34Z

Test build #113392 has finished for PR 26337 at commit 2f3d233.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public final class CalendarInterval implements Serializable, Ordered<CalendarInterval>

cloud-fan · 2019-11-08T02:20:16Z

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

 * The internal representation of interval type.
 */
-public final class CalendarInterval implements Serializable {
+public final class CalendarInterval implements Serializable, Ordered<CalendarInterval> {


is it possible to use java orderable? it's a java class

I should be able to use java comparable. I will try this.

cloud-fan · 2019-11-08T02:21:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

        SubtractTimestamps(Cast(l, TimestampType), r)
+
+      case b @ BinaryOperator(l @ CalendarIntervalType(), r @ NullType()) =>
+        b.withNewChildren(Seq(l, Cast(r, CalendarIntervalType)))


can we move it closer to other CalendarIntervalType cases?

SparkQA · 2019-11-08T05:36:19Z

Test build #113428 has finished for PR 26337 at commit d9cc157.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public final class CalendarInterval implements Serializable, Comparable<CalendarInterval>

yaooqinn · 2019-11-08T05:39:32Z

retest this please

maropu · 2019-11-08T09:03:41Z

retest this please

cloud-fan · 2019-11-08T09:07:58Z

retest this please

cloud-fan · 2019-11-08T09:09:17Z

sql/core/src/test/resources/sql-tests/inputs/interval.sql

+
+-- less than or equal
+select interval '1 minutes' < interval '1 hour';
+select interval '-1 day' >= interval '-23 hour';


nit: <= to match the comment

SparkQA · 2019-11-08T11:33:08Z

Test build #113454 has finished for PR 26337 at commit d9cc157.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public final class CalendarInterval implements Serializable, Comparable<CalendarInterval>

SparkQA · 2019-11-08T13:47:20Z

Test build #113459 has finished for PR 26337 at commit 5404d70.

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

cloud-fan · 2019-11-08T14:09:49Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala

      case Divide(l @ CalendarIntervalType(), r @ NumericType()) =>
        DivideInterval(l, r)

+      case b @ BinaryOperator(l @ CalendarIntervalType(), r @ NullType()) =>


This is a little hacky. Maybe we should introduce UnresolvedMultiply and UnresolvedDivide, so that we don't need to hack the type coercion rules.

We can try it in followup.

cloud-fan · 2019-11-08T14:10:38Z

sql/core/src/test/resources/sql-tests/inputs/interval.sql

@@ -0,0 +1,43 @@
+-- test for intervals


now we have a dedicated sql test file for interval, maybe we should put all interval related tests here. We can do it in followup

cloud-fan · 2019-11-08T14:45:23Z

thanks, merging to master!

### What changes were proposed in this pull request? As we are not going to follow ANSI to implement year-month and day-time interval types, it is weird to compare the year-month part to the day-time part for our current implementation of interval type now. Additionally, the current ordering logic comes from PostgreSQL where the implementation of the interval is messy. And we are not aiming PostgreSQL compliance at all. THIS PR will revert #26681 and #26337 ### Why are the changes needed? make interval type more future-proofing ### Does this PR introduce any user-facing change? there are new in 3.0, so no ### How was this patch tested? existing uts shall work Closes #27262 from yaooqinn/SPARK-30551. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

yaooqinn changed the title ~~[SPARK-29679][SQL] Make interval type support binary comparator~~ [SPARK-29679][SQL] Make interval type comparable and orderable Oct 31, 2019

MaxGekk reviewed Oct 31, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 3, 2019

cloud-fan reviewed Nov 5, 2019

View reviewed changes

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

cloud-fan reviewed Nov 5, 2019

View reviewed changes

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

cloud-fan reviewed Nov 5, 2019

View reviewed changes

yaooqinn added 4 commits November 5, 2019 14:44

[SPARK-29679][SQL] Make interval type camparable and orderable

e963e4a

add order ut

571bfe4

fix bug when microsecond field has not limit

853b993

add test for min/max

f03753c

fix null comp and regen golden file

2f3d233

cloud-fan reviewed Nov 8, 2019

View reviewed changes

yaooqinn added 3 commits November 8, 2019 11:16

mv closer

578ea9f

Merge branch 'master' into SPARK-29679

d872273

use java interface comparable

d9cc157

cloud-fan reviewed Nov 8, 2019

View reviewed changes

nit

5404d70

cloud-fan reviewed Nov 8, 2019

View reviewed changes

cloud-fan closed this in e026412 Nov 8, 2019

yaooqinn deleted the SPARK-29679 branch November 27, 2019 11:45

yaooqinn mentioned this pull request Jan 19, 2020

[SPARK-30551][SQL] Disable comparison for interval type #27262

Closed

MaxGekk mentioned this pull request Apr 28, 2020

SPARK-31447 Fix issue in ExtractIntervalPart expression #28222

Closed

	* @param daysPerMonth The number of days per one month. The default value is 31 days
	* per month. This value was taken as the default because it is used
	* in Structured Streaming for watermark calculations. Having 31 days
	* per month, we can guarantee that events are not dropped before
	* the end of any month (February with 29 days or January with 31 days).
	* @return Duration in the specified time units
	*/
	def getDuration(
	interval: CalendarInterval,
	targetUnit: TimeUnit,
	daysPerMonth: Int = 31): Long = {

	def getEpoch(interval: CalendarInterval): Decimal = {
	var result = interval.microseconds
	result += DateTimeUtils.MICROS_PER_DAY * interval.days
	result += MICROS_PER_YEAR * (interval.months / MONTHS_PER_YEAR)
	result += MICROS_PER_MONTH * (interval.months % MONTHS_PER_YEAR)
	Decimal(result, 18, 6)
	}

[SPARK-29679][SQL] Make interval type comparable and orderable #26337

[SPARK-29679][SQL] Make interval type comparable and orderable #26337

Uh oh!

Conversation

yaooqinn commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

MaxGekk left a comment

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 Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 4, 2019

Uh oh!

yaooqinn commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Nov 5, 2019

Uh oh!

yaooqinn commented Oct 31, 2019 •

edited

Loading

MaxGekk Nov 5, 2019 •

edited

Loading