Skip to content

feat: support divide_dt_interval with codegen dispatch - #4901

Open
peterxcli wants to merge 14 commits into
apache:mainfrom
peterxcli:feat/divide_dt_interval
Open

peterxcli wants to merge 14 commits into
apache:mainfrom
peterxcli:feat/divide_dt_interval

Conversation

@peterxcli

@peterxcli peterxcli commented Jul 12, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes: #3096.

Rationale for this change

This adds support for Spark's divide_dt_interval expression, 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?

  • Register DivideDTInterval with the JVM codegen dispatcher.
  • Update the expression support documentation.
  • Add SQL coverage for byte, short, integer, long, float, double, and decimal divisors, including wide decimals (DECIMAL(38, 18) and DECIMAL(38, 0)) as both columns and literals.
  • Cover interval literals, nulls, half-up microsecond rounding, negative values, division by zero, and the Long.MinValue / -1 overflow case.

(The dispatcher's DayTimeIntervalType input support — Arrow DurationVector handling, 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.
  • CI runs CometSqlFileTestSuite in 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 abce02b32dd842829965c292bb790eb84898335a with 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 used local[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=false to emulate the pre-PR unregistered-expression fallback; the after arm set it to true. 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

Projection Fallback (ms) Dispatch (ms) Speedup
Literal interval / INT column 292.5 277.0 1.06×
Literal interval / DOUBLE column 281.5 278.0 1.01×
Literal interval / DECIMAL(38,18) column 3,415.0 3,382.0 1.01×
Interval constructed from columns / INT column 462.5 382.0 1.21×
Mixed projection 1,269.0 748.5 1.70×

The varying-interval case evaluates make_dt_interval(days, hours, 0, seconds) / i. The mixed projection evaluates interval division alongside length(s), id + 1, and substring(s, 1, 4). Literal-interval cases use INTERVAL '1 02:03:04.500001' DAY TO SECOND with a varying divisor column.

Results at 1,048,576 rows
Projection Fallback (ms) Dispatch (ms) Speedup
Literal interval / INT column 72.5 72.0 1.01×
Literal interval / DOUBLE column 63.0 65.5 0.96×
Literal interval / DECIMAL(38,18) column 464.0 451.5 1.03×
Interval constructed from columns / INT column 87.0 81.5 1.07×
Mixed projection 184.5 137.0 1.35×

Plan and output validation: all 20 checks passed (five projections × two sizes × two arms).

  • Both arms retained the native Comet Parquet scan and a column-dependent DivideDTInterval.
  • Before: CometNativeScan → CometColumnarToRow → ProjectExec, with no Comet projection and zero dispatcher activity.
  • After: CometNativeScan → CometProjectExec → CometColumnarToRow, with no Spark projection or unexpected fallback and positive dispatcher counters.
  • Every timed iteration consumed the actual interval through queryExecution.toRdd and InternalRow.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.

@peterxcli peterxcli changed the title feat: support divide_dt_interval feat: support divide_dt_interval with codegen dispatch Jul 20, 2026
…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
@andygrove

Copy link
Copy Markdown
Member

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

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 Long.MinValue / -1 case.

A few things.

Map ordering in QueryPlanSerde

classOf[DivideDTInterval] -> CometDivideDTInterval is inserted between DateDiff and DateFormatClass. The surrounding entries are alphabetical (DateAdd, DateDiff, DateFormatClass, DateFromUnixDate, Days, ...), so this one is out of place. It belongs after Days. Small thing, but that map is long enough that keeping it sorted is the only way anyone finds anything in it.

Decimal divisor coverage stops at DECIMAL(10, 2)

The fixture uses a decimal(10,2) column and a DECIMAL(10, 2) literal. Spark's DivideDTInterval accepts any NumericType divisor, so DECIMAL(38, 18) and DECIMAL(38, 0) are both legal. Does the codegen dispatcher handle those, or is there an input-type limit that would make them fall back? A couple of rows with a wide decimal divisor would answer that, and if they do fall back, that is worth a line in expressions.md.

Testing on one Spark version

The description says the suite was run with -Pspark-3.5. DivideDTInterval and DayTimeIntervalType exist from 3.2 onward so I would expect this to be fine everywhere, but the codegen dispatcher's DurationVector handling is Comet's own code and Comet supports 3.4 through 4.1+. Did you run the fixture against 3.4 and 4.1 as well? If not it is worth doing before merge, since a per-version gap in the dispatcher's type handling would only show up in the [expressions] CI jobs.

Is there a performance number?

The rationale is compatibility rather than speed, which is fine. But the change moves DivideDTInterval from "falls back to Spark" to "runs through the dispatcher", and the dispatcher has its own per-batch cost. Do you know whether this is actually faster than the fallback for a realistic batch, or is the win purely that the surrounding operator stays native? If it is the latter, saying so in the description would set the right expectation.

peterxcli and others added 2 commits August 28, 2026 09:45
…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>
@peterxcli

Copy link
Copy Markdown
Member Author

Thanks for the review! Point by point:

  1. Map ordering — fixed; moved DivideDTInterval after Days.

  2. Wide decimals — the dispatcher handles all decimal precisions: CometBatchKernelCodegen.isSupportedDataType accepts any DecimalType, and inputs with precision > 18 read through a BigDecimal path on DecimalVector (precision ≤ 18 uses an unscaled-long fast path). No fallback, so no expressions.md caveat needed. I've added DECIMAL(38, 18) and DECIMAL(38, 0) divisors to the fixture — both as columns (to exercise the wide-decimal Arrow read path) and as literals — and it passes under checkSparkAnswerAndOperator, which fails on any fallback. (multiply_dt_interval.sql has the same DECIMAL(10, 2) cap; happy to extend it in a follow-up.)

  3. Spark versionsCometSqlFileTestSuite runs in the [expressions] CI job on all five 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 this fixture is exercised per Spark version before merge. Also, the dispatcher's DurationVector support isn't introduced here — it landed with multiply_dt_interval (feat: support multiply_dt_interval with codegen dispatch #4900) and feat: support interval codegen dispatch for nested values and native shuffle #4976 and is already covered per-version on main; this PR's diff is just the serde registration, fixture, and docs. I additionally ran the fixture locally against the default Spark 4.1 profile.

  4. Performance — no standalone speedup claim: the dispatcher runs Spark's own generated code per batch, so the expression itself is at parity. The win is that the enclosing operator stays native instead of forcing a columnar→row transition and operator fallback. I've updated the PR description to say so explicitly.

sunchao pushed a commit that referenced this pull request Aug 28, 2026
)

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>
@andygrove andygrove added enhancement New feature or request area:expressions Expression evaluation temporal expressions labels Sep 6, 2026

object CometMakeDTInterval extends CometCodegenDispatch[MakeDTInterval]

object CometDivideDTInterval extends CometCodegenDispatch[DivideDTInterval]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rich7420

rich7420 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@peterxcli thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation enhancement New feature or request temporal expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support Spark expression: divide_dt_interval

3 participants