andygrove commented on code in PR #5030: URL: https://github.com/apache/datafusion-comet/pull/5030#discussion_r3662245808
########## 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: Rebased onto main, so the rows now carry the Implementation column. On regenerating: I ran `GenerateDocs` to check my values rather than hand-guessing, and it agrees on every row this PR touches. `make_interval` resolves to `Codegen dispatch`; `timestampadd`, `timestampdiff` and `timediff` all get the em-dash placeholder, as you predicted, since `registerInternalExpression` writes to the `internal` registry rather than `FunctionRegistry.expressions`, which is what `buildFunctionNameToKind` iterates. So the value is uniform across profiles here, just uniformly the placeholder. I did not commit the generator output. On this checkout it also flipped about eight unrelated rows (`hour`, `minute`, `second`, `make_timestamp` to the placeholder; `<<`, `>>`, `>>>` to `Native`), which looks like main was last generated from a different profile tail. That churn does not belong in this PR, so the committed `expressions.md` diff is just the rows I edited. Happy to fold the regeneration in as a separate commit if you would rather have main resynced. ########## 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: Added. `MILLISECOND` and `DAYOFYEAR` are in the all-units block now, with a comment noting `DAYOFYEAR` shares the `case "DAY" | "DAYOFYEAR"` arm so the alias is proven to parse and dispatch. ########## 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: Added `MICROSECOND`, `MILLISECOND` and `QUARTER` in a second block, with `QUARTER` also exercised on literals. Agreed it was the risky one, being the only entry with its own arithmetic. -- 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]
