andygrove commented on code in PR #5030: URL: https://github.com/apache/datafusion-comet/pull/5030#discussion_r3662247104
########## 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: Added to both fixtures. `timestampadd.sql` has a spring-forward block, a fall-back block using the ambiguous `2024-11-03 01:30`, and `timestampadd(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.sql` has the matching cases, including `timestampdiff(HOUR, ...)` across spring-forward reporting 24 despite 23 elapsed hours. The comments in both files spell out why: local-time calendar arithmetic via `timestampAddInterval` on one side and `getLocalDateTime` plus `ChronoUnit.X.between` on the other. ########## 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: Added: `query expect_error(DATETIME_OVERFLOW)` over `timestampadd(YEAR, 1000000000, timestamp'2024-01-15 10:30:45')`, which overflows in `Math.multiplyExact(quantity, MONTHS_PER_YEAR)`. Confirmed it passes on 3.4, 3.5, 4.0 and 4.1, so the exception does cross out of the generated kernel with the condition name intact. Worth recording for anyone adding a similar case: this is version-sensitive. The `make_interval` ANSI overflow I added in the same pass could not match on `ARITHMETIC_OVERFLOW`, because 3.5 renders that one as a bare `"integer overflow. If necessary set ..."` with no condition prefix. That fixture matches on `overflow` instead. `DATETIME_OVERFLOW` happens to carry its prefix on every supported version. ########## 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: Added an NTZ column to `timestampadd.sql` with its own query block covering `HOUR`, `MONTH`, `DAY` and `MICROSECOND`, and an NTZ block in `timestampdiff.sql` to lock in the cast-up behavior. -- 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]
