SPARK-31447 Fix issue in ExtractIntervalPart expression

[sathyaprakashg]

What changes were proposed in this pull request?

ExtractIntervalPart change

ExtractIntervalPart expression get the interval parts information from CalendarInterval, which has three fields months, days and microseconds.

To get days interval ExtractIntervalDays expression simply returns the days field of provided CalendarInterval input.

If input is CalendarInterval(months=2, days=10, microseconds=0), it returns output as 10, but instead it should return total days in the interval. In this it has to return as 70 days.

CalendarInterval change

Another change I am proposing is equality comparision between two CalendarInterval object.
Right now, CalendarInterval(months=1, days=0, microseconds=0) and CalendarInterval(months=0, days=30, microseconds=0) are not equal.

I understand a month could have 30 or 31 days, but there should be way to compare two intervals even if they have different values in three fields but if both have same epoch it should be considered as equal

Change to equals method in CalendarInterval is not required to fix the issue in ExtractIntervalPart, but I feel we should fix how two CalendarInterval objects are compared in equals method.

Why are the changes needed?

Below sql statement return output as 0 instead of 14 days
SELECT EXTRACT(DAY FROM (cast('2020-01-15 00:00:00' as timestamp) - cast('2020-01-01 00:00:00' as timestamp)))

Below is why it returns wrong output

val start = Instant.parse("2019-01-01T00:00:00.000000Z")
val end = Instant.parse("2019-01-15T00:00:00.000000Z")

SubtractTimestamps(Literal(end), Literal(start)) expression returns CalendarInterval(months=0, days=0, microseconds=1209600000)

So, when evaluating below expression ExtractIntervalDays returns the value in days field in interval, which is zero, but correct answer is 14 days
ExtractIntervalDays(SubtractTimestamps(Literal(end), Literal(start)))

Does this PR introduce any user-facing change?

No

How was this patch tested?

• Additional + existing unit tests

[AmplabJenkins]

Can one of the admins verify this patch?

[sathyaprakashg]

@cloud-fan @MaxGekk @yaooqinn I am looking for help to review this PR created 2 weeks ago. Since you guys were involved in PR related to simillar change (https://issues.apache.org/jira/browse/SPARK-31469), I am tagging you guys to see if you can help it review it.

Since it is my first PR, please bear with me if I missed anything. I am happy to get guidance to improve it.

[MaxGekk]

@sathyaprakashg Please, take a look at the PRs
#26337
#27262

[sathyaprakashg]

@sathyaprakashg Please, take a look at the PRs
#26337
#27262

Thanks @MaxGekk for prompt reply. CalendarInterval change is not required to fix the issue. I can revert the proposed change for CalendarInterval change.

How does my proposed change for ExtractIntervalPart looks? If it looks good, I will update my PR to include only ExtractIntervalPart change

We need to change ExtractIntervalPart so that below query returns output as 14 instead of 0. Please refer Why are the changes needed? for more information

SELECT EXTRACT(DAY FROM (cast('2020-01-15 00:00:00' as timestamp) - cast('2020-01-01 00:00:00' as timestamp)))

[cloud-fan]

cc @yaooqinn can you take a look? This seems like a hard problem as we have a non-standard interval definition. It's interesting to see what results other systems return, like presto, hive, snowflake, etc.

[yaooqinn]

Checked PostgresSQL(not ANSI interval type) and presto(ANSI), both of them return proper days

[yaooqinn]

Presto returns day-time intervals only with timestamp subtractions, I think we can handle this change with our own CalendarInterval.

BTW, it seems both interval extraction and timestamp subtraction are newly added in 3.0.0? I am +1 for return both days and microseconds

[cloud-fan]

I think extracting days should return "interval.days + interval.microseconds / micros_per_day", to simulate the day-time interval semantic. But we shouldn't consider months in this case.

@yaooqinn can you send a PR to make this happen?

@sathyaprakashg we are trying hard to avoid assuming 1 month = 30 days. This makes interval not comparable, but we need to see how other systems implement it. I know pgsql assumes 1 month = 30 days, can you check other systems like presto, hive, snowflake, etc.?

[yaooqinn]

@cloud-fan, nice suggestion, I get your point. I will make a PR then.

[sathyaprakashg]

@cloud-fan Here are the output of month difference between two timestamps returned by different databases

SQL Server:
Statement: SELECT DATEDIFF(month, '2020-01-15 00:00:00', '2021-01-15 00:00:00')
Result : 12

Statement: SELECT DATEDIFF(month, '2020-01-16 00:00:00', '2021-01-15 00:00:00')
Result : 12

Statement: SELECT DATEDIFF(month, '2020-01-31 00:00:00', '2021-01-15 00:00:00')
Result : 12

Statement: SELECT DATEDIFF(month, '2020-02-01 00:00:00', '2021-01-15 00:00:00')
Result : 11

Presto:
Statement: SELECT date_diff('month', cast('2020-01-15 00:00:00' as timestamp),cast('2021-01-15 00:00:00' as timestamp) )
Result : 12

Statement: SELECT date_diff('month', cast('2020-01-16 00:00:00' as timestamp),cast('2021-01-15 00:00:00' as timestamp) )
Result : 11

Postgres:
Statement: SELECT EXTRACT(YEAR FROM age) * 12 + EXTRACT(MONTH FROM age) AS months_between FROM age(TIMESTAMP '2021-01-15 00:00:00', TIMESTAMP '2020-01-15 00:00:00') AS t(age)
Result : 12

Statement: SELECT EXTRACT(YEAR FROM age) * 12 + EXTRACT(MONTH FROM age) AS months_between FROM age(TIMESTAMP '2021-01-15 00:00:00', TIMESTAMP '2020-01-15 00:00:00') AS t(age)
Result : 11

MySQL:
Statement: SELECT TIMESTAMPDIFF(MONTH, '2020-01-15 00:00:00', '2021-01-15 00:00:00')
Result : 12

Statement: SELECT TIMESTAMPDIFF(MONTH, '2020-01-16 00:00:00', '2021-01-15 00:00:00')
Result : 11

In summary it looks none of the above database is assuming 30 days per day, since 365 days is not giving 12 months as result.

SQL Server takes the difference in month value of date regardless of day values.

But all other databases seems to be checking whether day of date1 is higher than or equal to day of date2 to check whether to consider that month or not.

Oracle and Hive have only MONTHS_BETWEEN function so I didn't include in analysis

[sathyaprakashg]

@cloud-fan
Spark gives 0 for the same query.
SELECT EXTRACT(MONTH FROM (cast('2021-01-15 00:00:00' as timestamp) - cast('2020-01-15 00:00:00' as timestamp)))

Simillar to getDays we need to adjust getMonths function to consider interval.microseconds as well. But assuming 30 DAYS per Month would wrong result.

Any suggestions?

[cloud-fan]

Can we use EXTRACT function consistenly in other systems? Otherwise it's hard to justify if the behavior of EXTRACT should follow MONTHS_BETWEEN or date_diff.

[sathyaprakashg]

@cloud-fan Unfortunatly, there is no date_diff to find month difference in hive so I used MONTHS_BETWEEN function

[cloud-fan]

Spark has MONTHS_BETWEEN function as well, and the behavior is the same as Hive's.

Can you look at MySQL, Oracle, SQL server, etc. if they support EXTRACT?

[sathyaprakashg]

@cloud-fan I updated SQL Server and MySQL also in my original comment. Hive and Oracle only support MONTHS_BETWEEN function.

Please let me know your suggestion on how to handle this in spark. If possible, I would like to take this opportunity to contribute to get this fixed in spark :)

[cloud-fan]

OK seems only pgsql has extract. Can we check the behavior of extract(month ...) in pgsql? I can't tell it from SELECT EXTRACT(YEAR FROM age) * 12 + EXTRACT(MONTH FROM age) AS months_between FROM ...

[sathyaprakashg]

Below is the output of postgresql.

SELECT EXTRACT(YEAR FROM age) year_part, 
EXTRACT(MONTH FROM age) month_part,
EXTRACT(DAY FROM age) day_part,
EXTRACT(hour FROM age) hour_part,
EXTRACT(minute FROM age) minute_part,
EXTRACT(second FROM age) second_part
FROM age(TIMESTAMP '2021-03-16 10:09:07', TIMESTAMP '2020-01-15 00:00:00') AS t(age)

  | year_part | month_part | day_part | hour_part | minute_part | second_part
| 1 | 2 | 1 | 10 | 9 | 7

select age(TIMESTAMP '2021-03-15 10:09:07', TIMESTAMP '2020-01-15 00:00:00')

age
420.10:09:07

Output translates to 420 days, 10 hours, 9 minutes and 7 seconds.

[cloud-fan]

The same query fails in Preso:

presto> select extract(month from interval '40' hour);
Query 20200428_193926_00006_ahn7x failed: Unexpected parameters (interval day to second) for function month. Expected: month(timestamp) , month(timestamp with time zone) , month(date) , month(interval year to month)

I don't have a good idea here as Spark's interval type is non-standard. Returning 0 seems reasonable as well.

[yaooqinn]

Below is the output of postgresql.

SELECT EXTRACT(YEAR FROM age) year_part, 
EXTRACT(MONTH FROM age) month_part,
EXTRACT(DAY FROM age) day_part,
EXTRACT(hour FROM age) hour_part,
EXTRACT(minute FROM age) minute_part,
EXTRACT(second FROM age) second_part
FROM age(TIMESTAMP '2021-03-16 10:09:07', TIMESTAMP '2020-01-15 00:00:00') AS t(age)

| year_part | month_part | day_part | hour_part | minute_part | second_part
| 1 | 2 | 1 | 10 | 9 | 7

select age(TIMESTAMP '2021-03-15 10:09:07', TIMESTAMP '2020-01-15 00:00:00')

age
420.10:09:07

Output translates to 420 days, 10 hours, 9 minutes and 7 seconds.

I think the problem is not about the extract function but how the age expression it relies on.

The extract function itself should follow the natural rules of the Georgian Calendar and achieve its semantics based on the output of extract-source.

If the age expression can result in a proper CalendarInterval value that contains the right months, days, and microseconds, then the extract function can work as you expected.

The spark project currently doesn't have an age function here, so I have it quickly implemented in my personal project https://github.com/yaooqinn/spark-func-extras

Then we can get the proper results as you did above

https://github.com/yaooqinn/spark-func-extras/blob/master/src/test/scala/org/apache/spark/sql/extra/PostgreSQLExtensionsTest.scala#L127-L150

test("age") {
    checkAnswer(
      sql("select age(timestamp '2001-04-10',  timestamp '1957-06-13')"),
      Seq(Row(new CalendarInterval(525, 28, 0)))
    )

    checkAnswer(
      sql(
        """
          |SELECT EXTRACT(YEAR FROM age) year_part,
          |EXTRACT(MONTH FROM age) month_part,
          |EXTRACT(DAY FROM age) day_part,
          |EXTRACT(hour FROM age) hour_part,
          |EXTRACT(minute FROM age) minute_part,
          |EXTRACT(second FROM age) second_part
          |FROM values (age(TIMESTAMP '2021-03-16 10:09:07', TIMESTAMP '2020-01-15 00:00:00')) AS t(age)
          |""".stripMargin),
        Seq(Row(1, 2, 1, 10, 9, Decimal(7000000, 8, 6).toJavaBigDecimal))
    )

    checkAnswer(
      sql("select age(date 'today')"),
      Seq(Row(new CalendarInterval(0, 0, 0)))
    )
  }

In other words, if the age function has a result type of Integer just represents the year, it will quickly fail the extract with analysis error here.

One more thing, if you really want to justify a month with 30 days, there are justifyDays, justifyHours and justifyInterval functions in that project

[sathyaprakashg]

@cloud-fan @yaooqinn for great discussion on this.

@yaooqinn has fixed getDays in the below PR and for getMonths we can agree on keeping the existing behavior as appropriate.

#28396

I have another quick question, before we close this PR.

SubtractTimestamps sems to be always populating only microseconds of CalendarInterval

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

Line 2242 in b7cde42

new CalendarInterval(0, 0, end.asInstanceOf[Long] - start.asInstanceOf[Long])

Whereas SubtractDates seems to be populating months along with days field.

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

Line 2261 in b7cde42

DateTimeUtils.subtractDates(leftDays.asInstanceOf[Int], rightDays.asInstanceOf[Int])

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

Line 940 in ea525fe

new CalendarInterval(months, days, 0)

My question is whether we should change SubtractTimestamps also to populate appropraiate months and days fields as well in CalendarInterval or whether you guys feel existing implementation is appropriate

[sathyaprakashg]

@cloud-fan @yaooqinn
I would like to get your opinion on my question related to SubtractTimestamps in above discussion.

Based on that we can make a decision to close this PR

[yaooqinn]

Hi @sathyaprakashg
This could be an API change and may cause performance regression for this operator. IMHO, we may need much stronger evidence to make the change, e.g. SQL standard, behaviors for timestamp - timestamp from other modern DBMS systems as much as possible. Otherwise, returning only micros part may be spark-specific but efficient and causes no ambiguity.