andygrove commented on PR #5051: URL: https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5147440247
@peterxcli the timezone question is settled in c25c7b32f — thanks for checking Spark's implementations, your recommendation was the right call and for a reason I had missed. The distinction that matters: Spark's Arrow cache can build `TimestampType` from `conf.sessionLocalTimeZone` **safely** because its RecordBatch is deliberately schema-less, so the label is never persisted and is reconstructed on read. `CometCachedBatch` stores a full IPC stream *including the schema*, so for us that label really does get written down — which makes a mutable session value materially worse here than the analogy with Spark suggests. So the row write path now passes `CometArrowStream.NATIVE_TIMEZONE`, matching the columnar path, which already did. Both of your predictions held: - **No values move** — Spark stores timestamps as micros since the Unix epoch regardless of session timezone. The test checks values *and* their `CAST(ts AS STRING)` rendering against Spark under two non-UTC session timezones. - **`TimestampNTZType` needed nothing** — `Utils.toArrowType` maps it to `Timestamp(MICROSECOND, null)` whatever is passed in. Small bonus: the closure no longer needs the session timezone, so it captures nothing derived from `conf`. That removes a live footgun — the original had to hoist `val sessionTz` outside `mapPartitions`, and when I moved the read inside while testing, it NPEd in `ConfigEntry.readString` on the executor. The new test caches a row-based plan (local `Seq`, so it goes through `convertInternalRowToCachedBatch`) under `America/Los_Angeles` and `Asia/Kolkata`, decodes the cached batches back through the serializer, and asserts the Arrow field timezone. I verified it is not vacuous: reverting the one-line change fails it with `got [America/Los_Angeles]`. All 20 tests in `CometInMemoryCacheSuite` pass on spark-3.5 (1 canceled, the pre-existing `isSpark40Plus` gate). That clears every inline thread on this PR except the AQE test coverage, which we agreed can be a follow-up. -- 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]
