andygrove commented on PR #5638:
URL:
https://github.com/apache/datafusion-comet/pull/5638#issuecomment-5529580991
_LLM-assisted: this reply and the changes it describes were written with
Claude Code._
@sunchao benchmarks added, both kinds.
`native/spark-expr/benches/iceberg_transforms.rs` (fa0158d) is the reusable
microbenchmark: every transform over every supported type, with and without
nulls, plus the dictionary-encoded string shape.
`CometIcebergSystemFunctionBenchmark` is the matched Comet-versus-Iceberg-JVM
comparison — the Comet-off arm *is* Iceberg's own implementation, since Spark
binds each function as a `StaticInvoke` of the class under
`org.apache.iceberg.spark.functions` and codegen calls it per row. The data
stays in Parquet rather than an Iceberg table so both arms scan identically and
only the transform differs, and `runExpressionBenchmark` warns loudly if the
Comet plan is not fully native or if the optimizer folded the expression away;
neither warning fired for any case.
Apple M3 Max, JDK 17, Spark 4.1, release native build, 1,048,576 rows, best
of five:
```
Spark (Iceberg JVM) Comet Relative
bucket(int) 31 ms 22 ms 1.4X
bucket(long) 28 ms 21 ms 1.3X
bucket(decimal(38,10)) 110 ms 30 ms 3.7X
bucket(string, dictionary) 33 ms 19 ms 1.7X
bucket(string) 101 ms 73 ms 1.4X
bucket(binary) 71 ms 50 ms 1.4X
bucket(date) 25 ms 21 ms 1.2X
bucket(timestamp) 34 ms 22 ms 1.6X
truncate(int) 23 ms 19 ms 1.2X
truncate(long) 24 ms 19 ms 1.2X
truncate(decimal(38,10)) 134 ms 72 ms 1.9X
truncate(string, dictionary) 42 ms 25 ms 1.7X
truncate(string) 95 ms 74 ms 1.3X
truncate(binary) 60 ms 47 ms 1.3X
years(date) 38 ms 18 ms 2.0X
years(timestamp) 53 ms 20 ms 2.7X
months(date) 36 ms 18 ms 2.0X
months(timestamp) 52 ms 20 ms 2.6X
days(date) 21 ms 18 ms 1.2X
days(timestamp) 59 ms 19 ms 3.0X
hours(timestamp) 60 ms 19 ms 3.1X
```
No case regresses. Dictionary expansion *was* a regression before this round
— @jordepic called it — and is fixed in 1622503; numbers on that thread.
Correctness is unchanged: the same queries run under
`checkSparkAnswerAndOperator` in the suite, so every one of these is compared
against the Iceberg JVM result as well as timed against it.
On the two P2s: the calendar-range one is fixed in 71e03be, with the
reference values taken from running Iceberg's `DateTimeUtil` on a JVM. The
decimal-intermediate one I have answered on its thread — short version, a
`Decimal128(p, s)` array has no encoding for the JVM intermediate, so the
kernel has to pick a side; I picked the side Spark materializes and documented
the difference in the user guide.
On CI: all six `[scans]` jobs pass on the current head, 3.4 and 3.5
included. I have replied on the fixture thread about the 4.0/4.1 missing file —
I could not fetch those logs either, so I cannot give you a confirmed cause,
only that the test has been green on 4.0/4.1/4.2 since f8db9b5.
--
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]
