0lai0 opened a new pull request, #6204:
URL: https://github.com/apache/datafusion-comet/pull/6204
## 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.
--
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]
