andygrove commented on PR #5638: URL: https://github.com/apache/datafusion-comet/pull/5638#issuecomment-5531945848
_LLM-assisted: this reply and the changes it describes were written with Claude Code._ @sunchao the decimal P2 is resolved rather than mitigated: decimal `truncate` now reports `Unsupported` (0a57b6a), so it falls back to Spark and there is no early nulling left to document. @jordepic's `Incompatible` route would also have worked; I chose `Unsupported` because the config-name problem you identified is real, and fixing it properly changes what `spark.comet.expression.StaticInvoke.enabled` covers for four unrelated static invokes. `Unsupported` needs no key. I did take your docs point — `CometStaticInvoke.getUnsupportedReasons()` now aggregates the per-function notes so the generated compatibility page carries it, with a test pinning that. The sort claim is dropped rather than narrowed. @jordepic corrected himself and you were right to push: `RowOrdering.create` evaluates the sort expression per comparison, so there is no `UnsafeRowWriter` in the sort path at all. Benchmark: every case now runs over a no-null column and a column with one null in eight, and each case's output is compared row by row between the two engines over the same corpus that is then timed, before it is timed. 40 cases, all equal. `truncate` on a decimal is gone from the set since it no longer has a native path. Apple M3 Max, JDK 17, Spark 4.1, release native build, 1,048,576 rows, best of five, as `no-null / with-null`: ``` bucket(int) 1.3X / 1.4X truncate(int) 1.1X / 1.3X years(date) 1.9X / 2.3X bucket(long) 1.2X / 1.4X truncate(long) 1.2X / 1.4X years(ts) 2.6X / 2.4X bucket(dec) 3.6X / 3.1X truncate(str_dict) 1.6X / 1.8X months(date) 2.0X / 2.2X bucket(str_dict) 1.6X / 1.7X truncate(str) 1.2X / 1.3X months(ts) 2.4X / 2.5X bucket(str) 1.3X / 1.3X truncate(bin) 1.6X / 1.3X days(date) 1.1X / 1.4X bucket(bin) 1.4X / 1.3X days(ts) 2.9X / 2.9X bucket(date) 1.2X / 1.4X hours(ts) 3.1X / 3.0X bucket(ts) 1.5X / 1.6X ``` On isolating transform cost from the scan: I have not done that, and you are right that a shared Parquet input does not achieve it. Both arms read the same files with the same partitioning, so the delta is scan-plus-projection, and these ratios are a floor on the expression-level gain rather than a measurement of it. The kernel-level criterion benchmark is the isolated measurement and the query-level one is the end-to-end number; I have described them that way rather than adding a third arm, which would need a change to the shared benchmark base. -- 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]
