Conversation
…o feat/divide_dt_interval
…rval # Conflicts: # docs/source/user-guide/latest/expressions.md # spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenInput.scala # spark/src/main/scala/org/apache/comet/serde/datetime.scala # spark/src/main/scala/org/apache/comet/udf/codegen/CometScalaUDFCodegen.scala
This is a clean, small change and routing through the codegen dispatcher is clearly the right choice for interval division, where the rounding and overflow rules are fiddly enough that reimplementing them natively would be asking for divergence. The SQL fixture covers a good spread: all seven divisor types, literals on both sides, half-up microsecond rounding, and the A few things. Map ordering in
Decimal divisor coverage stops at The fixture uses a Testing on one Spark version The description says the suite was run with Is there a performance number? The rationale is compatibility rather than speed, which is fine. But the change moves |
…rage - Move DivideDTInterval after Days to keep the temporalExpressions map's alphabetical prefix intact. - Cover DECIMAL(38, 18) and DECIMAL(38, 0) divisors in the SQL fixture, both as columns (DecimalVector slow path for precision > 18) and as literals. No fallback: the codegen dispatcher supports all decimal precisions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…at/divide_dt_interval
|
Thanks for the review! Point by point:
|
) MultiplyDTInterval routes through the JVM codegen dispatcher since #4900 (CometMultiplyDTInterval extends CometCodegenDispatch), so "Interval multiplication falls back" is no longer accurate. Only YearMonth (MultiplyYMInterval) and Calendar interval multiplication still fall back. Mirror the wording of the `/` row introduced by #4901. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval] | ||
|
|
||
| object CometDivideDTInterval extends CometCodegenDispatch[DivideDTInterval] |
There was a problem hiding this comment.
Could you add a before/after benchmark for a projection using this expression? That would show whether avoiding fallback offsets the dispatcher and Arrow conversion costs. Please verify the fallback/dispatch plans and consume the interval result.
|
@peterxcli thanks for the patch! |
…rval # Conflicts: # docs/source/user-guide/latest/expressions.md
Which issue does this PR close?
Closes: #3096.
Rationale for this change
This adds support for Spark's
divide_dt_intervalexpression, allowing DayTime interval division to execute within Comet through the JVM codegen dispatcher instead of falling back to Spark.Using Spark's generated code preserves Spark-compatible rounding, null handling, divide-by-zero errors, and overflow behavior.
The dispatcher evaluates Spark's generated code per batch while keeping the enclosing projection in Comet. Local projection benchmarks below measured the largest benefit when neighboring expressions also stayed native: 1.70× faster for a mixed projection at 8,388,608 rows. Double and wide-decimal division were near parity at that size; these measurements do not establish a general speedup for the division function in isolation.
What changes are included in this PR?
DivideDTIntervalwith the JVM codegen dispatcher.DECIMAL(38, 18)andDECIMAL(38, 0)) as both columns and literals.Long.MinValue / -1overflow case.(The dispatcher's
DayTimeIntervalTypeinput support — ArrowDurationVectorhandling, interval literal serialization, and nested codegen inputs — landed on main separately via #4900 and #4976; this PR only registers the expression and adds coverage.)How are these changes tested?
./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite divide_dt_interval"— run locally against Spark 3.5 (-Pspark-3.5 -Pscala-2.12) and the default Spark 4.1 profile.CometSqlFileTestSuitein the[expressions]job on all supported profiles (Spark 3.4/JDK 11/Scala 2.12, 3.5/JDK 17/Scala 2.13, 4.0/JDK 21, 4.1/JDK 17, 4.2/JDK 17), so the fixture is exercised per Spark version.Projection benchmark
Run on 2026-09-15 against PR head
abce02b32dd842829965c292bb790eb84898335awith a local benchmark harness, addressing the projection benchmark request.Setup: Spark 4.1.3 / Scala 2.13.17, Temurin JDK 17.0.20.1, AMD EPYC 9654 on a shared Linux host, native release build with
RUSTFLAGS=-Ctarget-cpu=native. Spark usedlocal[1], an 8 GiB heap, 8,192-row batches, ANSI enabled, and AQE disabled. JVM active processor count was 4; Comet worker/max-blocking threads were 2/4. Input was deterministic Snappy Parquet with signed, nonzero divisor columns and nulls.Comparison: both arms used the same build. The before arm set
spark.comet.expression.DivideDTInterval.enabled=falseto emulate the pre-PR unregistered-expression fallback; the after arm set it totrue. The global codegen dispatcher stayed enabled in both arms. These are controlled fallback/dispatch measurements, rather than two separately built commits.Timing: 2 seconds of warmup per arm, then at least 5 measured iterations and at least 2 seconds of measured time. Arms ran in before / after / after / before order. Each reported time is the mean of the two corresponding arm averages; speedup is fallback time divided by dispatch time. Timings include planning, scanning, projection, final row conversion, and result consumption. Corpus generation and Spark reference checks were outside timing.
8,388,608 rows
The varying-interval case evaluates
make_dt_interval(days, hours, 0, seconds) / i. The mixed projection evaluates interval division alongsidelength(s),id + 1, andsubstring(s, 1, 4). Literal-interval cases useINTERVAL '1 02:03:04.500001' DAY TO SECONDwith a varying divisor column.Results at 1,048,576 rows
Plan and output validation: all 20 checks passed (five projections × two sizes × two arms).
DivideDTInterval.CometNativeScan → CometColumnarToRow → ProjectExec, with no Comet projection and zero dispatcher activity.CometNativeScan → CometProjectExec → CometColumnarToRow, with no Spark projection or unexpected fallback and positive dispatcher counters.queryExecution.toRddandInternalRow.getLong(0), accumulating row count, null count, sum, and XOR of microseconds. The mixed case also consumed its neighboring outputs. Each checksum was checked against an untimed Spark-only reference, so a count-only action could not prune the division.Small differences should be read against repeat variation on this shared host. For example, the first 1-million-row integer fallback averaged 79 ms and its repeat averaged 66 ms. The 8-million-row mixed-projection result was clearer: fallback repeats averaged 1,265/1,273 ms versus 742/755 ms for dispatch. These are warmed projection measurements; cold-start compilation and larger downstream pipelines were not measured.