feat: route timestamp_seconds decimal, byte and short input through codegen dispatch - #6204
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem:
timestamp_secondsforced projections back to Spark for decimal, byte, and short inputs. - Design approach: Adding
CodegenDispatchFallbacklets those inputs use Spark’s generated code inside the Comet pipeline. - Correctness / compatibility analysis: Spark sources and expression tests for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 confirm the same conversion semantics. Decimal conversion uses
longValueExact(), preserving precision and overflow errors. Reviewed null handling, conditional evaluation, and timestamp representation. No introduced P1/P2 issues found within this review. - Key design decisions: Reuses the existing dispatcher without introducing another conversion implementation or abstraction. Integer, long, float, and double inputs retain their native routing.
- Implementation sketch: One serde mixin, updated support documentation, SQL coverage for dispatch/native/fallback paths and errors, and two benchmark cases.
- Behavioral changes worth calling out: Newly supported inputs keep surrounding operators eligible for Comet execution. Disabling the dispatcher preserves Spark fallback. The author reports roughly 1–2 ms overhead for standalone dispatch cases. Those measurements were not independently reproduced.
- Suggested improvements: None substantiated at the P1/P2 threshold.
Reviewed full SHA 4dbdc57d1020c2fe44aa050784d759f72b7a3457 against base 67803a7a422c44de07af1e5d25c1dbeae8df68d4, covering the complete five-file PR diff from their merge base. There are no stacked prerequisite commits. The PR is not a draft. Snapshot and live review, issue-comment, inline-comment, and thread checks found no existing discussion or blockers.
Routed skills: review-comet-pr and review-comet-expression-pr.
Exact-head CI: Labeling passed. Comet CI, CodeQL, and Check PR Title report action_required. Comet CI has no executed jobs, so there is no CI test verdict.
Validation: cargo build --locked --offline and diff whitespace checks passed. Spark 4.1.3 reference execution of both SQL fixtures completed 48 successful queries and six expected errors across the configured dictionary variants. This validates Spark reference behavior, not Comet dispatch. The focused Comet JVM suite could not start because Maven bootstrap failed with UnknownHostException: repo.maven.apache.org; installed Maven also rejected the checkout’s configuration. Spark SQL CI suites, Iceberg suites, and benchmarks were not run locally.
Which issue does this PR close?
Closes #5588.
Part of #5572.
Rationale for this change
timestamp_secondshad a native path only for int, long, float and double input. Decimal, byte and short input returnedUnsupported, so the whole projection fell back to Spark. All three types passCometBatchKernelCodegen.isSupportedDataType, so the JVM codegen dispatcher can run Spark's owndoGenCodefor them inside the Comet pipeline instead.Note that Spark's decimal branch does not round, contrary to the issue text. It computes
c.toJavaBigDecimal().multiply(1000000).longValueExact(), identical in 3.4.3, 3.5.8, 4.0.1 and 4.1.1, which raisesRounding necessaryon a nonzero digit past microsecond precision andOverflowoutside the long range. The dispatcher reproduces both because it runs the same generated code.What changes are included in this PR?
CometSecondsToTimestampmixes inCodegenDispatchFallback.getSupportLevelis unchanged: int, long, float and double stay native, the rest now dispatch instead of falling back.getUnsupportedReasonslists the decimal, byte and short inputs, andexpressions.mdmarks the rowHybrid.CometCodegenDispatchBenchmarkgainstimestamp_seconds(decimal)andtimestamp_seconds(tinyint)cases plus the two corpus columns they read.How are these changes tested?
timestamp_seconds.sqladds tinyint, smallint,decimal(10,0),decimal(20,6)anddecimal(38,18)columns and literal arguments asexpect_dispatch, marks the existing int, long and double queriesexpect_nativeand adds a float column, and pins both errors withexpect_error. A raising decimal row placed in an unselectedCASE WHEN,IForcoalescebranch confirms the dispatcher does not raise where Spark would not. The file setsspark.comet.exec.scalaUDF.codegen.enabled=trueso the sentinel check applies and the error queries cannot pass vacuously. Newtimestamp_seconds_fallback.sqlchecks that the three types fall back with the expected reason when the dispatcher is off.With the mixin removed, all 3 fail.
./mvnw spotless:checkpasses.Performance
make benchmark-org.apache.spark.sql.benchmark.CometCodegenDispatchBenchmark, Apple M5, JDK 17.0.18, Spark 4.1, 1048576 rows, best time:On a projection whose only expression is
timestamp_seconds, dispatch is not faster than the Spark fallback: across three runs it lands 1 to 2 ms above it while the repeat baseline moves 0 to 1 ms, so the bridge costs a few percent at most and is not always separable from noise. The gain is in the existing mixed cases, where one unhandled expression used to cost the whole projection and, for the aggregate, everything above it. This PR is about not losing the rest of the plan to one decimal argument.Running the benchmark with and without the two new corpus columns moved every existing arm by at most 2 ms, except
to_time(fmt)where all four arms moved together, which is drift.Not covered:
run-*label before it queues.LIMIT, semi and anti join filtering, and empty input were checked by hand on Spark 4.1 and 3.4 and matched Spark, but are not in the test file.