andygrove commented on code in PR #5864: URL: https://github.com/apache/datafusion-comet/pull/5864#discussion_r3997162212
########## 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 Review Comment: These prove the answers match and the operator stays native, but not that any of it went through the dispatcher, which is the thing the PR is claiming. `expect_dispatch(...)` already exists for exactly this and is used in `math/round.sql` and `string/upper.sql`. `CometSqlFileTestSuite` accepts it as a file sentinel, so it is a strictly stronger assertion than a plain `query` rather than an extra one to maintain. Could the lead query in each file become `query expect_dispatch(subtractdates)` and so on? It matters most for the legacy `ts - ts` decline. That branch is only correct because default mode is known to hit the dispatcher's duration writer, and right now nothing pins that. It is also the assertion that would settle the `date + interval` point I left on the expressions page, since `date + make_dt_interval(...)` and `date + INTERVAL '1' DAY` take different routes and these fixtures cannot tell them apart. `timestamp_add_interval.sql` is the awkward one, since the name changes from `timeadd` to `timestampaddinterval` at 4.1, so leaving that file on a plain `query` seems fine. ########## docs/source/user-guide/latest/expressions.md: ########## @@ -415,8 +415,8 @@ The type-name conversion functions (`bigint`, `binary`, `boolean`, `date`, `deci | --- | --- | --- | --- | | `%` | ✅ | Native | | | `*` | ✅ | Native | DayTime interval multiplication routes through the JVM codegen dispatcher; YearMonth and Calendar interval multiplication fall back | -| `+` | ✅ | Native | | -| `-` | ✅ | Native | | +| `+` | ✅ | Native | Adding a calendar, year-month or day-time interval to a date or timestamp routes through the JVM codegen dispatcher | Review Comment: The `+` and `-` notes are broader than what actually happens for a day-granular interval on a date. Spark rewrites `(DateType, DayTimeIntervalType(DAY, DAY))` to `DateAdd(l, ExtractANSIIntervalDays(r))`, in `BinaryArithmeticWithDatetimeResolver` on 4.0 and 4.1 and in `Analyzer.ResolveBinaryArithmetic` on 3.4 and 3.5, and `CometDateAdd` serdes that natively. So `date + INTERVAL '30' DAY` never reaches the dispatcher, and that is the spelling most real queries use, TPC-DS included. Could the notes carve it out? Something like "A calendar or year-month interval on a date, and any interval on a timestamp, route through the JVM codegen dispatcher. `date +/- INTERVAL '<n>' DAY` is rewritten to `date_add` and stays native. A finer day-time interval on a date is cast to timestamp and dispatched." The `*` row two lines up already splits by interval type, so it would read consistently. The other direction is worth a thought as well. For a non-foldable DAY-precision interval column, `ExtractANSIIntervalDays` has no serde, so the projection falls back rather than dispatching. ########## spark/src/main/scala/org/apache/comet/serde/datetime.scala: ########## @@ -997,6 +997,37 @@ 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] Review Comment: Reading this right above the `CometSubtractTimestamps` guard, the obvious question is why this one does not need the same branch when its legacy result is also a `CalendarIntervalType`. The answer is in `subtract_dates.sql` but not here, and it is the invariant whoever adds the next calendar-interval-producing serde will need. Worth a line? Something like "Legacy mode returns a `CalendarIntervalType`, but `DateTimeUtils.subtractDates` always sets microseconds to 0, so the dispatcher's `multiplyExact` cannot overflow and both modes dispatch." I checked that on 3.4.3, 3.5.8, 4.0.1 and 4.1.3 and it holds on all four. -- 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]
