andygrove commented on code in PR #5864:
URL: https://github.com/apache/datafusion-comet/pull/5864#discussion_r3993859282
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -997,6 +997,33 @@ object CometTimestampAdd extends
CometCodegenDispatch[TimestampAdd]
object CometTimestampDiff extends CometCodegenDispatch[TimestampDiff]
+// Date and timestamp interval arithmetic. `timestamp + day-time or calendar
interval` resolves
+// to `TimeAdd` on Spark 3.4 through 4.0 and to `TimestampAddInterval` on
4.1+, so that serde
+// lives in the version shims.
+object CometDateAddInterval extends CometCodegenDispatch[DateAddInterval]
+
+object CometDateAddYMInterval extends CometCodegenDispatch[DateAddYMInterval]
+
+object CometTimestampAddYMInterval extends
CometCodegenDispatch[TimestampAddYMInterval]
+
+object CometSubtractDates extends CometCodegenDispatch[SubtractDates]
+
+object CometSubtractTimestamps extends
CometCodegenDispatch[SubtractTimestamps] {
+ private val legacyIntervalReason =
Review Comment:
This is the same `Math.multiplyExact(interval.microseconds, 1000L)` limit
that `CometMakeInterval` documents forty lines up, and that one links #5279.
Could this reason string carry the link too? Otherwise when #5279 lands and
CalendarInterval crosses the boundary losslessly, there is nothing here
pointing at the branch that became removable.
Worth a sentence on why the two take opposite approaches to the same bug, as
well. `MakeInterval` keeps dispatching and documents the limit as a compatible
note, this one declines the whole legacy mode. I think declining is right,
since `MakeInterval` only overflows on extreme arguments whereas `ts - ts` can
produce an arbitrary span from ordinary-looking data, and legacy interval mode
is off by default so almost nobody pays for it. But someone comparing the two
in the same file will wonder.
##########
spark/src/test/resources/sql-tests/expressions/datetime/subtract_dates.sql:
##########
@@ -0,0 +1,65 @@
+-- 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 - date resolves to SubtractDates and runs through the codegen
dispatcher so results
+-- match Spark exactly. The output type follows
spark.sql.legacy.interval.enabled: a
+-- DayTimeIntervalType(DAY) by default, a CalendarIntervalType in legacy mode.
+-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true
+-- Config: spark.comet.shuffle.mode=native
+-- ConfigMatrix: spark.sql.legacy.interval.enabled=false,true
+
+statement
+CREATE TABLE test_subtract_dates(d1 date, d2 date, k int) USING parquet
+
+-- the 2300 rows span about 330 years, past the 292-year limit of a nanosecond
long, but the
+-- day count of a date difference never touches that field
+statement
+INSERT INTO test_subtract_dates VALUES
+ (date'2024-03-15', date'2024-01-01', 1),
+ (date'2024-01-01', date'2024-03-15', 1),
+ (date'2024-02-29', date'2023-02-28', 2),
+ (date'1969-12-31', date'1970-01-02', 2),
+ (date'2024-06-01', date'2024-06-01', 3),
+ (date'1900-01-01', date'2100-12-31', 3),
+ (date'2300-01-01', date'1970-01-01', 6),
+ (date'1970-01-01', date'2300-01-01', 6),
+ (NULL, date'2024-01-01', 4),
+ (date'2024-01-01', NULL, 4),
+ (NULL, NULL, 5)
+
+-- column - column in both directions, covering negative and zero spans
+query
+SELECT d1, d2, d1 - d2, d2 - d1 FROM test_subtract_dates
+
+-- literal on either side
+query
+SELECT d1 - date'2024-01-01', date'2024-01-01' - d2 FROM test_subtract_dates
+
+-- all-literal operands (constant folding is disabled by the test suite). A
NULL literal operand
+-- is left out: NullPropagation folds it to a null interval literal, and the
native literal
Review Comment:
This omission already has a tracking issue, #5058, filed under the interval
EPIC #5061. Could the comment cite it, here and in the matching one in
`subtract_timestamps.sql`? Then whoever fixes #5058 has a grep target for the
fixtures that can be extended once it lands, and the "will get its own issue"
line in the description can go.
##########
spark/src/main/spark-3.x/org/apache/comet/serde/CometTimeAdd.scala:
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.
+ */
+
+package org.apache.comet.serde
+
+import org.apache.spark.sql.catalyst.expressions.TimeAdd
+
+/**
+ * `timestamp + day-time or calendar interval` resolves to `TimeAdd` on Spark
3.4 through 4.0 and
+ * runs through the codegen dispatcher. Spark 4.1 renames it to
`TimestampAddInterval`.
+ */
+object CometTimeAdd extends CometCodegenDispatch[TimeAdd]
Review Comment:
This file and the `spark-4.0` copy are byte-identical, same blob hash. The
description says no source root covers exactly 3.4 through 4.0, but I think
`shims.minorPlusVerSrc` is that root: it is `spark-none` on 3.4, 3.5 and 4.0
and `spark-4.1+` on 4.1 and 4.2, and `spark/pom.xml` already adds
`src/main/${shims.minorPlusVerSrc}` as a source directory. Giving that property
a real directory name on the 3.x and 4.0 profiles would let this live in one
place.
If reworking the build isn't worth it for 28 lines, the other way out is to
drop the named object and register `new CometCodegenDispatch[TimeAdd]` inline
in the three shim maps. It is a concrete class, so that compiles. Either beats
two copies that can drift.
--
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]
