sunchao commented on PR #5457: URL: https://github.com/apache/datafusion-comet/pull/5457#issuecomment-5443927497
Updated in [23ae9bb95](https://github.com/apache/datafusion-comet/commit/23ae9bb95896b464eb6c5957a521cc1d473d64df). @andygrove I checked each suggestion against the cast boundary and added changes for the avoidable copies and fixed-zone aliases. 1. **Named-zone correctness.** I am retaining the Spark dispatcher, with Spark row execution when dispatch is disabled. An in-range/out-of-range chrono check cannot preserve Spark results: Spark successfully casts `+262143-01-01` in `America/Los_Angeles` to `8210266905600000000` microseconds, although chrono cannot represent that date. Returning overflow or TRY null would be incorrect. There is also an in-range counterexample: on `2101-07-04`, the pinned chrono-tz version uses `-08:00` for Los Angeles while Java uses `-07:00`. The SQL regression now covers that date as well as the wide values. I added and ran the same Parquet-backed column benchmark on exact base `bd7dc601` and revised head `c54a5310`: 1,048,576 ordinary dates, 1% nulls, batch size 8,192, Spark 4.1.3/JDK 17, release native libraries, `local[1]`, AQE off, and four fresh JVMs in base/head/head/base order. For Los Angeles, the per-JVM query means were 131/130 ms on base and 137/156 ms with head dispatch (12.3% higher average); with dispatch disabled they were 128/128 ms on base, still native, and 149/162 ms with head Spark row projection (21.5% higher average). UTC stayed native and its average query means fell 39–45%. These are scan + cast + noop-sink timings for this workload, not isolated kernel costs or a general deployment estimate. The PR body includes the routes and raw per-run numbers. 2. **Nested widening copies.** Added a conservative whitelist for signed integer widenings and unchanged fixed-width children, including structs composed of those leaves. Nullable structs and sparse nullable lists/maps can reuse input buffers only when their children satisfy that whitelist. Nested lists/maps and unchanged strings/binary keep the existing compaction path, which prevents the added reuse path from allocating or retaining an unbounded hidden payload. Lists/maps with unused backing values or dense null rows still compact; the density check counts visible child values as well as null parent rows. Checked casts continue to mask hidden children in legacy, ANSI, and TRY modes. Tests compare logical results with the existing normalization and verify buffer reuse, offsets, null parents, map keys, empty slices, visible errors, and compaction of 65,536 hidden values/bytes through four nested shapes. Whole-cast identity remains the existing no-op; the new guard only bounds the added reuse path. Separately, the native reuse optimization was compared with unmodified PR head `8b072abb`, not the PR base. The 13 targeted nullable `Int32` to `Int64` struct and sparse list/map shapes used 12–92% less batch time. All 46 shapes include no-null, dense-null, slice, large-hidden-list, and checked-DATE controls. Six initial 1.8–3.3% control increases did not reproduce in opposite-order repeats (−3.6% to +0.14%); no reproducible regression was found among those measured shapes. 3. **LargeList/list views.** Spark `ArrayType` becomes Arrow `List` at the SQL serialization boundary. Recursive SQL casts support `List` to `List`; differing unsupported list-like types already reach the outer explicit unsupported-cast error. Returning an array from the preparation helper is not an admission decision. I have not expanded the supported type set in this fix. 4. **Timezone aliases.** Fixed aliases now resolve through Spark's `DateTimeUtils.getZoneId` and serialize as `UTC` or a canonical signed minute offset. This keeps `GMT`, `Etc/UTC`, `Z`, `UTC+00:00`, `GMT+05:30`, and `Etc/GMT+8` native, including inside nested casts. The old regex accepted a safe subset of the native parser; the confirmed problem was unnecessary fallback, rather than an admitted spelling that failed parsing. Checking only fixed rules would be unsafe: chrono rejects some original alias strings and parses `+05:30:15` as `+05:30`, dropping the seconds. Subminute offsets therefore remain on Spark. Added parity coverage for all 2,161 minute offsets, both signed zero spellings, aliases, subminute exclusions, and offset-induced overflow. 5. **DST helper comment.** `resolve_local_datetime` still documents gap/ambiguity handling and explicitly selects the pre-transition offset in its gap branch. Its remaining callers convert NTZ timestamps; DATE now uses Spark's own start-of-day rules. I kept that documentation rather than moving a DATE-specific explanation onto a different caller. Final validation against the revised release library: 690 native expression/common unit tests and 28 focused Spark 4.1.3 date-cast/error tests passed; all-target Clippy and the full JVM reactor style checks passed. An earlier full cast/error run had 184 passes and one failure in a newly added test that expected native execution for the already unsupported direct `ARRAY<DATE>` to `ARRAY<TIMESTAMP>` shape. I corrected that test to the supported array-of-struct shape and reran all 28 affected tests successfully; I have not claimed a subsequent full-suite run. -- 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]
