andygrove opened a new pull request, #6340:
URL: https://github.com/apache/datafusion-comet/pull/6340
## Which issue does this PR close?
No issue. This follows #6337, which adds the timezone handling page to the
contributor guide, and is part of the timezone EPIC #6335.
## Rationale for this change
#6337 adds `docs/source/contributor-guide/timezones.md`, but the review
skills don't point at it. A review of a datetime expression, a cast, a scan or
the JVM/native boundary has no reason to open the page.
Most of the bugs in #6335 are ones a review could have caught:
- `timestamp_seconds` returns a `TimestampType` result with no timezone
label (#6328)
- `date_trunc` stamps the session timezone on its result, so comparisons
fail in `Etc/UTC` sessions, where it runs natively (#6330)
- `CometDays` reads `SQLConf.get.sessionLocalTimeZone` instead of the
timezone on the expression (#6333)
These pass any test that only projects the result.
No single area skill owns timezone handling. It spans the serde, the native
kernels, the scans, the JVM/native boundary and the codegen dispatcher. So the
main check goes in `review-comet-pr`, and the area skills get short items for
the parts that are specific to them.
## What changes are included in this PR?
- **`review-comet-pr`**: step 1 says to read `timezones.md` for any PR that
deals with timestamps or the session timezone. Step 5 gets a "Timestamps and
timezones" check. It covers how to tell whether a PR is affected (including a
`grep` over the diff), the problems to look for first, and which parts of the
page a PR can make stale.
- **`review-comet-expression-pr`**:
- The page goes in the guide table.
- New checklist items cover serializing `expr.timeZoneId`, labelling
`TimestampType` results `"UTC"`, DataFusion datetime functions that evaluate in
UTC, and UTC fast paths.
- The timezone test item now follows the page's testing section.
- "Timestamp result mislabelled" is added to the common findings.
- **`review-comet-ffi-pr`**: timestamps cross the boundary unconverted, and
a new JVM producer labels them with `CometArrowStream.NATIVE_TIMEZONE`.
- **`review-comet-iceberg-write-pr`**: timestamp partition values are UTC.
iceberg-rust's `years` and `months` follow the column's timezone label
(apache/iceberg-rust#3142), so they are only right because
`decorate_batch_with_field_ids` relabels `timestamptz` columns `+00:00` before
partitioning.
This should merge after #6337, because the skills point at the page it adds.
## How are these changes tested?
Documentation only. `prettier --check` passes on the changed files. I
checked every name the skills cite against `main`:
`CometArrowStream.NATIVE_TIMEZONE`, `array_with_timezone`, `is_utc_timezone` in
`extract_date_part.rs`, the Iceberg transform kernels, and
`decorate_batch_with_field_ids` along with where it runs relative to
partitioning. I also tried the diff search on recent commits. It matches three
datetime commits, and it doesn't match three shuffle commits or #6250.
--
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]
