Skip to content

feat: route timestamp_seconds decimal, byte and short input through codegen dispatch - #6204

Merged
sunchao merged 1 commit into
apache:mainfrom
0lai0:feat-5588-timestamp-seconds-codegen-fallback
Sep 27, 2026
Merged

sunchao merged 1 commit into
apache:mainfrom
0lai0:feat-5588-timestamp-seconds-codegen-fallback

Conversation

@0lai0

@0lai0 0lai0 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5588.

Part of #5572.

Rationale for this change

timestamp_seconds had a native path only for int, long, float and double input. Decimal, byte and short input returned Unsupported, so the whole projection fell back to Spark. All three types pass CometBatchKernelCodegen.isSupportedDataType, so the JVM codegen dispatcher can run Spark's own doGenCode for 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 raises Rounding necessary on a nonzero digit past microsecond precision and Overflow outside the long range. The dispatcher reproduces both because it runs the same generated code.

What changes are included in this PR?

  • CometSecondsToTimestamp mixes in CodegenDispatchFallback. getSupportLevel is unchanged: int, long, float and double stay native, the rest now dispatch instead of falling back.
  • getUnsupportedReasons lists the decimal, byte and short inputs, and expressions.md marks the row Hybrid.
  • CometCodegenDispatchBenchmark gains timestamp_seconds(decimal) and timestamp_seconds(tinyint) cases plus the two corpus columns they read.

How are these changes tested?

timestamp_seconds.sql adds tinyint, smallint, decimal(10,0), decimal(20,6) and decimal(38,18) columns and literal arguments as expect_dispatch, marks the existing int, long and double queries expect_native and adds a float column, and pins both errors with expect_error. A raising decimal row placed in an unselected CASE WHEN, IF or coalesce branch confirms the dispatcher does not raise where Spark would not. The file sets spark.comet.exec.scalaUDF.codegen.enabled=true so the sentinel check applies and the error queries cannot pass vacuously. New timestamp_seconds_fallback.sql checks that the three types fall back with the expected reason when the dispatcher is off.

./mvnw [-Pspark-3.4|-Pspark-3.5|-Pspark-4.0|] test -Dtest=none \
  '-Dsuites=org.apache.comet.CometSqlFileTestSuite timestamp_seconds'
Result: 3 tests passed on each of the four profiles.

With the mixin removed, all 3 fail. ./mvnw spotless:check passes.

Performance

make benchmark-org.apache.spark.sql.benchmark.CometCodegenDispatchBenchmark, Apple M5, JDK 17.0.18, Spark 4.1, 1048576 rows, best time:

timestamp_seconds(decimal)                     dispatch off: 36   dispatch: 37   Spark: 51   (repeat: 37)
timestamp_seconds(tinyint)                     dispatch off: 14   dispatch: 15   Spark: 22   (repeat: 14)
mixed projection (existing case)               dispatch off: 116  dispatch: 70   Spark: 153  (repeat: 116)
group by dispatch (existing case)              dispatch off: 49   dispatch: 44   Spark: 46   (repeat: 49)

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:

  • The Spark SQL and Iceberg suites have not been run. This changes a serde, so a committer should apply the matching run-* label before it queues.
  • Benchmark numbers are from one machine.
  • 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.

@github-actions github-actions Bot added enhancement New feature or request area:expressions Expression evaluation labels Sep 24, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

  • Prior state and problem: timestamp_seconds forced projections back to Spark for decimal, byte, and short inputs.
  • Design approach: Adding CodegenDispatchFallback lets 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.

@sunchao
sunchao added this pull request to the merge queue Sep 27, 2026
Merged via the queue into apache:main with commit 605051a Sep 27, 2026
38 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

timestamp_seconds falls back to Spark for decimal, byte and short input

2 participants