-
Notifications
You must be signed in to change notification settings - Fork 343
feat: support timestampadd and timestampdiff via codegen dispatch #5030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- DATE_ADD and DATE_DIFF joined the timestampadd/timestampdiff grammar rules in Spark 3.5. | ||
| -- With a datetimeUnit keyword as the first argument they parse to TimestampAdd/TimestampDiff | ||
| -- and run through the codegen dispatcher, not to the two-argument DateAdd/DateDiff. The | ||
| -- two-argument forms stay native and are covered here so both spellings are pinned together. | ||
| -- See timestampadd.sql and timestampdiff.sql for the unit and DST coverage. | ||
| -- MinSparkVersion: 3.5 | ||
| -- Config: spark.sql.session.timeZone=America/Los_Angeles | ||
| -- Config: spark.comet.exec.scalaUDF.codegen.enabled=true | ||
|
|
||
| statement | ||
| CREATE TABLE test_date_add_unit(ts timestamp, d date, q int) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_date_add_unit VALUES | ||
| (timestamp'2024-01-15 10:30:45', date'2024-01-15', 3), | ||
| (timestamp'2024-01-31 23:00:00', date'2024-01-31', 1), | ||
| (timestamp'2024-02-29 12:00:00', date'2024-02-29', -5), | ||
| (NULL, NULL, 1), | ||
| (timestamp'2024-06-15 00:00:00', date'2024-06-15', NULL) | ||
|
|
||
| -- unit form parses to TimestampAdd | ||
| query | ||
| SELECT | ||
| date_add(DAY, q, ts), | ||
| date_add(MONTH, 1, ts), | ||
| date_add(HOUR, 6, ts), | ||
| date_add(MICROSECOND, 500, ts) | ||
| FROM test_date_add_unit | ||
|
|
||
| -- unit form parses to TimestampDiff | ||
| query | ||
| SELECT | ||
| date_diff(DAY, ts, timestamp'2024-07-01 00:00:00'), | ||
| date_diff(MONTH, ts, timestamp'2024-07-01 00:00:00'), | ||
| date_diff(HOUR, ts, timestamp'2024-07-01 00:00:00'), | ||
| date_diff(QUARTER, ts, timestamp'2024-07-01 00:00:00') | ||
| FROM test_date_add_unit | ||
|
|
||
| -- the two-argument forms are unaffected and stay on DateAdd / DateDiff | ||
| query | ||
| SELECT | ||
| date_add(d, q), | ||
| date_diff(d, date'2024-07-01') | ||
| FROM test_date_add_unit |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- TIMEDIFF was added to the timestampdiff grammar rule in Spark 4.0, so it parses to the | ||
| -- same TimestampDiff node and runs through the codegen dispatcher. See timestampdiff.sql | ||
| -- for the unit and DST coverage that applies to every spelling. | ||
| -- MinSparkVersion: 4.0 | ||
| -- Config: spark.sql.session.timeZone=America/Los_Angeles | ||
| -- Config: spark.comet.exec.scalaUDF.codegen.enabled=true | ||
|
|
||
| statement | ||
| CREATE TABLE test_timediff(a timestamp, b timestamp) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_timediff VALUES | ||
| (timestamp'2024-01-01 00:00:00', timestamp'2024-03-15 12:30:00'), | ||
| (timestamp'2024-03-15 12:30:00', timestamp'2024-01-01 00:00:00'), | ||
| (NULL, timestamp'2024-01-01 00:00:00'), | ||
| (timestamp'2024-01-01 00:00:00', NULL) | ||
|
|
||
| query | ||
| SELECT | ||
| timediff(YEAR, a, b), | ||
| timediff(MONTH, a, b), | ||
| timediff(DAY, a, b), | ||
| timediff(HOUR, a, b), | ||
| timediff(MICROSECOND, a, b) | ||
| FROM test_timediff | ||
|
|
||
| -- literal arguments (constant folding is disabled by the test suite) | ||
| query | ||
| SELECT | ||
| timediff(HOUR, timestamp'2024-03-09 12:00:00', timestamp'2024-03-10 12:00:00'), | ||
| timediff(QUARTER, timestamp'2024-01-01 00:00:00', timestamp'2024-08-15 00:00:00') |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- timestampadd runs through the codegen dispatcher so results match Spark exactly. | ||
| -- America/Los_Angeles is pinned so the DST cases below straddle real transitions. | ||
| -- Config: spark.sql.session.timeZone=America/Los_Angeles | ||
| -- Config: spark.comet.exec.scalaUDF.codegen.enabled=true | ||
|
|
||
| statement | ||
| CREATE TABLE test_timestampadd(ts timestamp, ts_ntz timestamp_ntz, q int) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_timestampadd VALUES | ||
| (timestamp'2024-01-15 10:30:45', timestamp_ntz'2024-01-15 10:30:45', 3), | ||
| (timestamp'2024-01-31 23:00:00', timestamp_ntz'2024-01-31 23:00:00', 1), | ||
| (timestamp'2024-02-29 12:00:00', timestamp_ntz'2024-02-29 12:00:00', 12), | ||
| (timestamp'2024-12-31 23:59:59', timestamp_ntz'2024-12-31 23:59:59', 2), | ||
| (timestamp'1970-01-01 00:00:00', timestamp_ntz'1970-01-01 00:00:00', -5), | ||
| (NULL, NULL, 1), | ||
| (timestamp'2024-06-15 00:00:00', timestamp_ntz'2024-06-15 00:00:00', NULL) | ||
|
|
||
| -- column quantity across a range of units, including month-end and leap-day rollover | ||
| query | ||
| SELECT timestampadd(HOUR, q, ts) FROM test_timestampadd | ||
|
|
||
| query | ||
| SELECT timestampadd(MONTH, q, ts) FROM test_timestampadd | ||
|
|
||
| -- every unit accepted by DateTimeUtils.timestampAdd. DAYOFYEAR shares a case arm with DAY, | ||
| -- so it is covered here to prove the alias both parses and dispatches. | ||
| query | ||
| SELECT | ||
| timestampadd(YEAR, 1, ts), | ||
| timestampadd(QUARTER, 1, ts), | ||
| timestampadd(WEEK, 2, ts), | ||
| timestampadd(DAY, -10, ts), | ||
| timestampadd(DAYOFYEAR, -10, ts), | ||
| timestampadd(MINUTE, 90, ts), | ||
| timestampadd(SECOND, 30, ts), | ||
| timestampadd(MILLISECOND, 1500, ts), | ||
| timestampadd(MICROSECOND, 500, ts) | ||
| FROM test_timestampadd | ||
|
Comment on lines
+46
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
|
|
||
| -- TIMESTAMP_NTZ input. TimestampAdd.dataType is timestamp.dataType, so this yields an NTZ | ||
| -- output, which compiles a separate kernel and resolves zoneIdForType to UTC. | ||
| query | ||
| SELECT | ||
| timestampadd(HOUR, q, ts_ntz), | ||
| timestampadd(MONTH, q, ts_ntz), | ||
| timestampadd(DAY, 1, ts_ntz), | ||
| timestampadd(MICROSECOND, 500, ts_ntz) | ||
| FROM test_timestampadd | ||
|
|
||
| -- the grammar routes TIMESTAMPADD and DATEADD to the same TimestampAdd node whenever the | ||
| -- first argument is a datetimeUnit keyword, so the alias spelling must land on this serde | ||
| -- rather than on the two-argument DateAdd. DATE_ADD joined the rule in Spark 3.5 and is | ||
| -- covered by date_add_unit_alias.sql. | ||
| query | ||
| SELECT | ||
| dateadd(DAY, q, ts), | ||
| dateadd(MONTH, 1, ts), | ||
| dateadd(HOUR, 6, ts) | ||
| FROM test_timestampadd | ||
|
|
||
| -- DST boundaries. timestampadd goes through timestampAddInterval, which does calendar | ||
| -- arithmetic on local time (.atZone(zoneId).plusDays(...)), so adding a day across the | ||
| -- spring-forward transition advances the local clock by one day rather than by 24 hours. | ||
| query | ||
| SELECT | ||
| timestampadd(DAY, 1, timestamp'2024-03-09 12:00:00'), | ||
| timestampadd(HOUR, 24, timestamp'2024-03-09 12:00:00'), | ||
| timestampadd(DAY, 1, timestamp'2024-11-02 12:00:00'), | ||
| timestampadd(HOUR, 24, timestamp'2024-11-02 12:00:00') | ||
|
|
||
| -- fall back: 2024-11-03 01:30 is an ambiguous local time | ||
| query | ||
| SELECT | ||
| timestampadd(HOUR, 1, timestamp'2024-11-03 00:30:00'), | ||
| timestampadd(HOUR, 1, timestamp'2024-11-03 01:30:00'), | ||
| timestampadd(MINUTE, 90, timestamp'2024-11-03 00:45:00') | ||
|
|
||
| -- spring forward: 2024-03-10 02:30 is a nonexistent local time | ||
| query | ||
| SELECT | ||
| timestampadd(HOUR, 1, timestamp'2024-03-10 01:30:00'), | ||
| timestampadd(MINUTE, 45, timestamp'2024-03-10 01:30:00'), | ||
| timestampadd(DAY, 1, timestamp'2024-03-09 02:30:00') | ||
|
|
||
| -- literal arguments (constant folding is disabled by the test suite) | ||
| query | ||
| SELECT | ||
| timestampadd(HOUR, 3, timestamp'2024-01-01 10:00:00'), | ||
| timestampadd(MONTH, 1, timestamp'2024-01-31 00:00:00'), | ||
| timestampadd(YEAR, 1, timestamp'2024-02-29 00:00:00') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added: Worth recording for anyone adding a similar case: this is version-sensitive. The |
||
|
|
||
| -- DateTimeUtils.timestampAdd maps ArithmeticException and DateTimeException to | ||
| -- timestampAddOverflowError, which must cross out of the generated kernel unchanged. | ||
| query expect_error(DATETIME_OVERFLOW) | ||
| SELECT timestampadd(YEAR, 1000000000, timestamp'2024-01-15 10:30:45') | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- timestampdiff runs through the codegen dispatcher so results match Spark exactly. | ||
| -- America/Los_Angeles is pinned so the DST cases below straddle real transitions. | ||
| -- Config: spark.sql.session.timeZone=America/Los_Angeles | ||
| -- Config: spark.comet.exec.scalaUDF.codegen.enabled=true | ||
|
|
||
| statement | ||
| CREATE TABLE test_timestampdiff(a timestamp, b timestamp, a_ntz timestamp_ntz, b_ntz timestamp_ntz) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_timestampdiff VALUES | ||
| (timestamp'2024-01-01 00:00:00', timestamp'2024-03-15 12:30:00', timestamp_ntz'2024-01-01 00:00:00', timestamp_ntz'2024-03-15 12:30:00'), | ||
| (timestamp'2024-03-15 12:30:00', timestamp'2024-01-01 00:00:00', timestamp_ntz'2024-03-15 12:30:00', timestamp_ntz'2024-01-01 00:00:00'), | ||
| (timestamp'2024-01-31 00:00:00', timestamp'2024-02-29 00:00:00', timestamp_ntz'2024-01-31 00:00:00', timestamp_ntz'2024-02-29 00:00:00'), | ||
| (timestamp'2020-02-29 00:00:00', timestamp'2024-02-29 00:00:00', timestamp_ntz'2020-02-29 00:00:00', timestamp_ntz'2024-02-29 00:00:00'), | ||
| (NULL, timestamp'2024-01-01 00:00:00', NULL, timestamp_ntz'2024-01-01 00:00:00'), | ||
| (timestamp'2024-01-01 00:00:00', NULL, timestamp_ntz'2024-01-01 00:00:00', NULL) | ||
|
|
||
| -- whole-unit differences are truncated toward zero, matching Spark | ||
| query | ||
| SELECT | ||
| timestampdiff(YEAR, a, b), | ||
| timestampdiff(MONTH, a, b), | ||
| timestampdiff(WEEK, a, b), | ||
| timestampdiff(DAY, a, b), | ||
| timestampdiff(HOUR, a, b), | ||
| timestampdiff(MINUTE, a, b), | ||
| timestampdiff(SECOND, a, b) | ||
| FROM test_timestampdiff | ||
|
Comment on lines
+37
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
|
|
||
| -- the remaining entries of timestampDiffMap. QUARTER is the only entry with arithmetic of | ||
| -- its own (MONTHS.between(...) / 3, integer division toward zero). | ||
| query | ||
| SELECT | ||
| timestampdiff(QUARTER, a, b), | ||
| timestampdiff(MILLISECOND, a, b), | ||
| timestampdiff(MICROSECOND, a, b) | ||
| FROM test_timestampdiff | ||
|
|
||
| -- TIMESTAMP_NTZ inputs. inputTypes is Seq(TimestampType, TimestampType), so NTZ is cast up. | ||
| query | ||
| SELECT | ||
| timestampdiff(YEAR, a_ntz, b_ntz), | ||
| timestampdiff(MONTH, a_ntz, b_ntz), | ||
| timestampdiff(HOUR, a_ntz, b_ntz), | ||
| timestampdiff(MICROSECOND, a_ntz, b_ntz) | ||
| FROM test_timestampdiff | ||
|
|
||
| -- the grammar routes TIMESTAMPDIFF and DATEDIFF to the same TimestampDiff node whenever the | ||
| -- first argument is a datetimeUnit keyword, so the alias spelling must land on this serde | ||
| -- rather than on the two-argument DateDiff. DATE_DIFF joined the rule in Spark 3.5 and is | ||
| -- covered by date_add_unit_alias.sql. | ||
| query | ||
| SELECT | ||
| datediff(DAY, a, b), | ||
| datediff(MONTH, a, b), | ||
| datediff(HOUR, a, b) | ||
| FROM test_timestampdiff | ||
|
|
||
| -- DST boundaries. timestampdiff converts both sides with getLocalDateTime and then calls | ||
| -- ChronoUnit.X.between on the local values, so a day spanning the spring-forward transition | ||
| -- still reports 24 hours even though only 23 real hours elapse. | ||
| query | ||
| SELECT | ||
| timestampdiff(HOUR, timestamp'2024-03-09 12:00:00', timestamp'2024-03-10 12:00:00'), | ||
| timestampdiff(DAY, timestamp'2024-03-09 12:00:00', timestamp'2024-03-10 12:00:00'), | ||
| timestampdiff(HOUR, timestamp'2024-11-02 12:00:00', timestamp'2024-11-03 12:00:00'), | ||
| timestampdiff(DAY, timestamp'2024-11-02 12:00:00', timestamp'2024-11-03 12:00:00') | ||
|
|
||
| -- across the transition instants themselves | ||
| query | ||
| SELECT | ||
| timestampdiff(MINUTE, timestamp'2024-03-10 01:30:00', timestamp'2024-03-10 03:30:00'), | ||
| timestampdiff(SECOND, timestamp'2024-11-03 00:30:00', timestamp'2024-11-03 02:30:00'), | ||
| timestampdiff(MICROSECOND, timestamp'2024-03-10 01:59:59', timestamp'2024-03-10 03:00:00') | ||
|
|
||
| -- literal arguments (constant folding is disabled by the test suite) | ||
| query | ||
| SELECT | ||
| timestampdiff(MONTH, timestamp'2024-01-31 00:00:00', timestamp'2024-02-29 00:00:00'), | ||
| timestampdiff(HOUR, timestamp'2024-01-01 00:00:00', timestamp'2024-01-02 06:00:00'), | ||
| timestampdiff(QUARTER, timestamp'2024-01-01 00:00:00', timestamp'2024-08-15 00:00:00'), | ||
| timestampdiff(DAY, timestamp'2024-03-15 12:30:00', timestamp'2024-01-01 00:00:00') | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies to timestampdiff.sql:
Pinning
America/Los_Angelesonly pays off if a fixture straddles a transition. Both functions do calendar arithmetic on local time, not elapsed time: timestampadd goes throughtimestampAddInterval, which is.atZone(zoneId).plusDays(...).plus(micros)(SparkDateTimeUtils.scala:269-274), and timestampdiff converts both sides withgetLocalDateTimeand then callsChronoUnit.X.betweenon the local values (:745-747). Sotimestampdiff(HOUR, timestamp'2024-03-09 12:00:00', timestamp'2024-03-10 12:00:00')is 24 despite 23 real hours elapsing, andtimestampadd(DAY, 1, timestamp'2024-03-09 12:00:00')is a local plus-one-day rather than plus-24-hours. Those are precisely the results a chrono-based native implementation gets wrong, which is the stated reason for choosing the dispatcher, and neither fixture asserts them. Please add a spring-forward row, a fall-back row (2024-11-03 01:30 is ambiguous), and one landing on the nonexistent local hour,timestampadd(HOUR, 1, timestamp'2024-03-10 01:30:00').There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to both fixtures.
timestampadd.sqlhas a spring-forward block, a fall-back block using the ambiguous2024-11-03 01:30, andtimestampadd(HOUR, 1, timestamp'2024-03-10 01:30:00')landing on the nonexistent local hour, plus the plus-one-day versus plus-24-hours pairing on both transitions.timestampdiff.sqlhas the matching cases, includingtimestampdiff(HOUR, ...)across spring-forward reporting 24 despite 23 elapsed hours.The comments in both files spell out why: local-time calendar arithmetic via
timestampAddIntervalon one side andgetLocalDateTimeplusChronoUnit.X.betweenon the other.