peterxcli commented on PR #5771: URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5602022090
Thanks for checking the arithmetic against Spark's helpers, and for the `Etc/GMT+1` / `Europe/London` boundary in particular — those are the two that would quietly break the allowlist. All three points are addressed in bb8a04858, on top of a merge of `upstream/main`. **Which branch runs in CI.** You were right that nothing pinned it: `CometHour` takes its zone from `expr.timeZoneId`, which Spark fills from the session zone, so the branch followed whichever runner executed the suite. `hour.sql`, `minute.sql` and `second.sql` now carry `-- ConfigMatrix: spark.sql.session.timeZone=UTC,America/Los_Angeles`, and each gained a `1969-12-31 23:59:59` row in both the timestamp and the NTZ table so the Euclidean division is covered end to end. Each file now runs as two variants and both pass locally on Spark 3.5 / JDK 17. **Two benchmark files for the same three UDFs.** Also correct, and the reason is that `benches/extract_date_part.rs` landed upstream after my branch point — I had not merged since. Merged now, the new shapes folded into that file, and `extract_clock_fields.rs` deleted. It uses the shared `common/` helpers, sweeps both session zones so each dispatch branch is actually measured (the file previously only ever constructed `America/Los_Angeles`, so the timezone-aware fast path was unreachable), adds a dense null ratio alongside `NULL_RATIOS`, adds dictionary inputs at cardinality 8 and 1024, and generates instants spanning the epoch instead of a fixed 2020 base. Upstream's existing shape ids are preserved under the `la_session` prefix, with the full `ROW_COUNTS` sweep kept there and the additions at 8192 rows so the matrix stays affordable. **PR body and title.** Updated. The title no longer claims 23x — that figure came from a `ci`-profile run before the dense-null and dictionary fixes — and the tables now carry the post-fix numbers, with the shapes that still move the wrong way stated in the body rather than only in a thread reply, since the squash message is what survives. One thing I did not change but that is worth someone's attention: the comment block in `hour.sql` says the native implementation is `Incompatible` for `TimestampNTZType` per #3180 and is therefore routed through the codegen dispatcher. `CometHour` has no `getSupportLevel` override — it is plain `Compatible` — so NTZ does reach the native path and that comment looks stale. I left it alone rather than widen this PR, but it will mislead the next reader of that 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]
