mbutrovich commented on code in PR #5030:
URL: https://github.com/apache/datafusion-comet/pull/5030#discussion_r3658380739
##########
docs/source/user-guide/latest/expressions.md:
##########
@@ -284,6 +284,8 @@ The type-name conversion functions (`bigint`, `binary`,
`boolean`, `date`, `deci
| `timestamp_micros` | ✅ | |
| `timestamp_millis` | ✅ | |
| `timestamp_seconds` | ✅ | |
+| `timestampadd` | ✅ | Runs through the JVM codegen dispatcher |
Review Comment:
`timestampadd` and `timestampdiff` are not the only way to build these
nodes. The grammar routes `TIMESTAMPADD | DATEADD | DATE_ADD` to `TimestampAdd`
and `TIMESTAMPDIFF | DATEDIFF | DATE_DIFF` (plus `TIMEDIFF` on 4.0) to
`TimestampDiff` whenever the first argument is a `datetimeUnit` keyword
(`SqlBaseParser.g4:939-940` on branch-3.5, `:1158-1159` on branch-4.0). So
`dateadd(DAY, 1, ts)` is a `TimestampAdd`, not a `DateAdd`, and before this PR
it fell back.
That makes four existing rows in
`docs/source/user-guide/latest/expressions.md` misleading: `date_add` (`:257`),
`date_diff` (`:258`), `dateadd` (`:264`) and `datediff` (`:265`) all claim
`Native`, which is true only of the two-argument form. `timediff` has no row at
all. Since this PR is what makes the unit form work, it is the right place to
correct those and to add at least one query per fixture using an alias
spelling, so the parse path is proven to land on the same serde.
Worth noting so nobody adds a test that cannot fail: the string-literal unit
form is rejected at parse time, not runtime. `visitTimestampadd` throws
`invalidDatetimeUnitError` when `ctx.invalidUnit` is set (Spark
`AstBuilder.scala:6012-6021`), so `timestampadd('MONTH', 1, ts)` never reaches
Comet and needs no fixture.
##########
docs/source/user-guide/latest/expressions.md:
##########
@@ -284,6 +284,8 @@ The type-name conversion functions (`bigint`, `binary`,
`boolean`, `date`, `deci
| `timestamp_micros` | ✅ | |
| `timestamp_millis` | ✅ | |
| `timestamp_seconds` | ✅ | |
+| `timestampadd` | ✅ | Runs through the JVM codegen dispatcher |
+| `timestampdiff` | ✅ | Runs through the JVM codegen dispatcher |
Review Comment:
The `expressions.md` hunk here adds three-column rows, but main gained an
Implementation column in 376676c7a, which is the conflict. That column is
generated by `GenerateDocs` from the serde's trait mixins, so please regenerate
with `dev/generate-release-docs.sh` rather than hand-merging the rows. The
value will not be uniform across profiles: `TimestampAdd` and `TimestampDiff`
are absent from the `FunctionRegistry` on 3.4 and 3.5 (reachable only through
the grammar above) and are `registerInternalExpression` on 4.0
(`FunctionRegistry.scala:927-928`), and unregistered names get the placeholder.
##########
spark/src/test/resources/sql-tests/expressions/datetime/timestampadd.sql:
##########
@@ -0,0 +1,58 @@
+-- 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.
+-- Config: spark.sql.session.timeZone=America/Los_Angeles
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+
+statement
+CREATE TABLE test_timestampadd(ts timestamp, q int) USING parquet
+
+statement
+INSERT INTO test_timestampadd VALUES
+ (timestamp'2024-01-15 10:30:45', 3),
+ (timestamp'2024-01-31 23:00:00', 1),
+ (timestamp'2024-02-29 12:00:00', 12),
+ (timestamp'2024-12-31 23:59:59', 2),
+ (timestamp'1970-01-01 00:00:00', -5),
+ (NULL, 1),
+ (timestamp'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
+
+query
+SELECT
+ timestampadd(YEAR, 1, ts),
+ timestampadd(QUARTER, 1, ts),
+ timestampadd(WEEK, 2, ts),
+ timestampadd(DAY, -10, ts),
+ timestampadd(MINUTE, 90, ts),
+ timestampadd(SECOND, 30, ts),
+ timestampadd(MICROSECOND, 500, ts)
+FROM test_timestampadd
Review Comment:
`DateTimeUtils.timestampAdd` handles MICROSECOND, MILLISECOND, SECOND,
MINUTE, HOUR, DAY, DAYOFYEAR, WEEK, MONTH, QUARTER, YEAR (Spark
`DateTimeUtils.scala:670-708`). This block covers all of them except
MILLISECOND and DAYOFYEAR, and both are valid grammar keywords on every
supported version (`SqlBaseParser.g4:931-935` on branch-3.5, `:1150-1154` on
branch-4.0), so both are testable as written. DAYOFYEAR is worth adding because
it is an alias arm sharing a case with DAY (`case "DAY" | "DAYOFYEAR"`,
`:687`), so nothing currently proves the alias both parses and dispatches.
##########
spark/src/test/resources/sql-tests/expressions/datetime/timestampadd.sql:
##########
@@ -0,0 +1,58 @@
+-- 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.
+-- Config: spark.sql.session.timeZone=America/Los_Angeles
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+
+statement
+CREATE TABLE test_timestampadd(ts timestamp, q int) USING parquet
Review Comment:
`TimestampAdd.dataType` is `timestamp.dataType` (Spark
`datetimeExpressions.scala:3443`), so an NTZ input yields an NTZ output. That
is a different Arrow vector class in the kernel (`TimeStampMicroVector` instead
of `TimeStampMicroTZVector`, `CometBatchKernelCodegen.scala:71-72`), hence a
separately compiled kernel, and `zoneIdForType` resolves to UTC instead of the
session zone. This is a distinct code path rather than a variation, so please
add an NTZ column here. For timestampdiff, `inputTypes` is `Seq(TimestampType,
TimestampType)` (`datetimeExpressions.scala:3531`) so NTZ gets cast up; one
case is enough to lock that in.
##########
spark/src/test/resources/sql-tests/expressions/datetime/timestampadd.sql:
##########
@@ -0,0 +1,58 @@
+-- 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.
+-- Config: spark.sql.session.timeZone=America/Los_Angeles
Review Comment:
Same applies to timestampdiff.sql:
Pinning `America/Los_Angeles` only pays off if a fixture straddles a
transition. Both functions do calendar arithmetic on local time, not elapsed
time: timestampadd goes through `timestampAddInterval`, which is
`.atZone(zoneId).plusDays(...).plus(micros)` (Spark
`DateTimeUtils.scala:269-274`), and timestampdiff converts both sides with
`getLocalDateTime` and then calls `ChronoUnit.X.between` on the local values
(`:745-747`). So `timestampdiff(HOUR, timestamp'2024-03-09 12:00:00',
timestamp'2024-03-10 12:00:00')` is 24 despite 23 real hours elapsing, and
`timestampadd(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, timest
amp'2024-03-10 01:30:00')`.
##########
spark/src/test/resources/sql-tests/expressions/datetime/timestampdiff.sql:
##########
@@ -0,0 +1,51 @@
+-- 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.
+-- 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) USING parquet
+
+statement
+INSERT INTO test_timestampdiff 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'),
+ (timestamp'2024-01-31 00:00:00', timestamp'2024-02-29 00:00:00'),
+ (timestamp'2020-02-29 00:00:00', timestamp'2024-02-29 00:00:00'),
+ (NULL, timestamp'2024-01-01 00:00:00'),
+ (timestamp'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
Review Comment:
`timestampDiffMap` covers MICROSECOND, MILLISECOND, SECOND, MINUTE, HOUR,
DAY, WEEK, MONTH, QUARTER, YEAR (Spark `DateTimeUtils.scala:719-730`). This
block omits MICROSECOND, MILLISECOND and QUARTER. QUARTER is the one entry in
that map with arithmetic of its own (`ChronoUnit.MONTHS.between(...) / 3`,
integer division toward zero), so it is both the most likely to diverge and the
only one untested.
##########
spark/src/test/resources/sql-tests/expressions/datetime/timestampadd.sql:
##########
@@ -0,0 +1,58 @@
+-- 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.
+-- Config: spark.sql.session.timeZone=America/Los_Angeles
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+
+statement
+CREATE TABLE test_timestampadd(ts timestamp, q int) USING parquet
+
+statement
+INSERT INTO test_timestampadd VALUES
+ (timestamp'2024-01-15 10:30:45', 3),
+ (timestamp'2024-01-31 23:00:00', 1),
+ (timestamp'2024-02-29 12:00:00', 12),
+ (timestamp'2024-12-31 23:59:59', 2),
+ (timestamp'1970-01-01 00:00:00', -5),
+ (NULL, 1),
+ (timestamp'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
+
+query
+SELECT
+ timestampadd(YEAR, 1, ts),
+ timestampadd(QUARTER, 1, ts),
+ timestampadd(WEEK, 2, ts),
+ timestampadd(DAY, -10, ts),
+ timestampadd(MINUTE, 90, ts),
+ timestampadd(SECOND, 30, ts),
+ timestampadd(MICROSECOND, 500, ts)
+FROM test_timestampadd
+
+-- 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')
Review Comment:
`DateTimeUtils.timestampAdd` wraps its whole body in a try that maps
ArithmeticException and DateTimeException to `timestampAddOverflowError` (Spark
`DateTimeUtils.scala:709-713`), which raises error class `DATETIME_OVERFLOW`
(`QueryExecutionErrors.scala:2484-2492`). Nothing here reaches it, and this is
the only path in the PR where an exception has to cross out of the generated
kernel and surface as the same Spark error. Add `query
expect_error(DATETIME_OVERFLOW)` over something like `timestampadd(YEAR,
1000000000, timestamp'2024-01-15 10:30:45')`; the existing valid-input blocks
already satisfy the sentinel requirement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
