andygrove opened a new pull request, #5260: URL: https://github.com/apache/datafusion-comet/pull/5260
## Which issue does this PR close? Closes #3099. Part of the interval support epic #5061. Split out of #5030, which originally carried this alongside `timestampadd` and `timestampdiff`. Those two are unrelated to `make_interval` beyond sharing the codegen-dispatch mechanism, so they now travel separately. Note there is an alternative native implementation in #5039, which wires `make_interval` to DataFusion's `SparkMakeInterval`. The two approaches are mutually exclusive; see the trade-off below. ## Rationale for this change `MakeInterval` has no Comet handler today, so any query using `make_interval` falls the entire operator back to Spark. It is a regular expression, not `RuntimeReplaceable`, so it reaches serde directly. This PR routes it through the JVM codegen dispatcher rather than adding a native implementation. The dispatcher runs Spark's own generated code inside the native Comet pipeline, which keeps the operator native while guaranteeing bit-for-bit Spark compatibility. For `make_interval` specifically this covers three behaviors that are easy to diverge on natively: - the `seconds` argument is a `Decimal(18, 6)` scaled to microseconds, with its own overflow check; - `failOnError` (defaulting to `SQLConf.ansiEnabled`) chooses between raising `ARITHMETIC_OVERFLOW` and returning NULL, and it must be the same exception Spark raises; - the `years * 12` and `weeks * 7` products overflow `int` independently of one another. `make_interval` produces `CalendarIntervalType`, which Comet's columnar layer gained support for in #4898. With that in place the dispatcher can carry its output. The trade-off against #5039: codegen dispatch gives exact Spark semantics for free but runs JVM code per batch, so it is slower than a native kernel. A native implementation is the better end state if it can match Spark on the decimal scaling and both ANSI paths. This PR is the low-risk option and can be superseded. ## What changes are included in this PR? - `CometMakeInterval` codegen-dispatch serde in `datetime.scala`, registered in `QueryPlanSerde`'s `temporalExpressions` map. - `make_interval.sql` and `make_interval_ansi.sql` Comet SQL file tests. - The `make_interval` row in the Supported Spark Expressions guide. ## How are these changes tested? New Comet SQL file tests run each query through both Spark and Comet, verify the results match, and verify Comet executes the expression through the dispatcher rather than falling back. `make_interval.sql` (non-ANSI, so `failOnError` is false) covers: - all seven arguments as columns, and each of the shorter arities filled in by `MakeInterval`'s auxiliary constructors; - mixed literal/column arguments and all-literal arguments (the suite disables constant folding, so these still reach Comet); - negative and zero components, and values that carry across units (25 months, 80 minutes, 299.889987 seconds); - NULL propagation from nullable columns. It has to come from columns rather than NULL literals: `MakeInterval` is `NullIntolerant`, so `NullPropagation` rewrites any call with a literal NULL argument into a null interval literal that never reaches Comet. That literal currently fails natively, tracked separately by #5058. - overflow returning NULL when `years * 12` exceeds the int range. `make_interval_ansi.sql` pins `spark.sql.ansi.enabled=true` so `failOnError` is true, and asserts the overflow raises the same error Spark does for both the `years * 12` and `weeks * 7` products. Each error query sits alongside valid-input queries in the same file so the case cannot pass vacuously through a fallback. Verified green on Spark 3.5 and 4.1 locally, along with the pre-existing `calendar_interval.sql` fixture and the `explain comet` test in `CometExpressionSuite`, both of which use `make_interval`. -- 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]
