refactor!: share partition timestamp parsing

[DrakeLin]

Stacked PR

Use this link to review incremental changes.

• stack/kernel-partition-timezone-parser-refactor [Files changed] <- this PR
  • stack/kernel-partition-timezone-parsing [Files changed]

---

What changes are proposed in this pull request?

This PR moves TIMESTAMP partition parsing and timezone resolution out of the Arrow evaluator into an engine-independent Kernel module so scan parsing and MapToStruct can share one implementation.

The shared parser accepts the protocol's space-separated timestamp form and ISO 8601 values with an explicit offset. A connector-provided IANA timezone or canonical fixed offset applies only to the space-separated form; an explicit timestamp offset takes precedence. Scan-side TIMESTAMP parsing now uses this shared parser with its existing UTC default, while DATE and TIMESTAMP_NTZ retain their existing parsing behavior.

As part of sharing the implementation, the parser removes permissive cast-only formats from this path and safely rejects malformed configured offsets instead of panicking.

How was this change tested?

Tests cover protocol timestamp formats, configured fixed and named timezones, daylight-saving transitions, invalid inputs, MapToStruct execution, and scan parsing. The default-feature and no-declarative test suites pass, along with formatting, Clippy, docs, and the no-default kernel build.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.51%. Comparing base (9f18bef) to head (c0a7a80).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3341   +/-   ##
=======================================
  Coverage   90.50%   90.51%           
=======================================
  Files         255      255           
  Lines       91237    91280   +43     
  Branches    91237    91280   +43     
=======================================
+ Hits        82574    82618   +44     
+ Misses       5650     5649    -1     
  Partials     3013     3013           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Benchmark results: ✅ Pass

Summary: 🚀 0  ·  ✅ 13  ·  ☑️ 2  ·  🚧 0  ·  ❌ 0

Per-benchmark results (15 rows)

Test	Change	Base	PR
clustered/readMetadataLatestPredicate/serial	✅ 1.00x	100.4±1.93ms	100.0±1.73ms
crcLatest/snapshotLatest	✅ 1.00x	9.7±0.51ms	9.7±0.10ms
crcMissing/snapshotLatest	☑️ 1.01x slower	23.3±0.29ms	23.6±0.85ms
crcSlightlyStale/snapshotLatest	✅ 1.00x	10.6±0.64ms	10.6±0.47ms
crcVeryStale/snapshotLatest	✅ 1.01x faster	16.0±0.15ms	15.9±0.14ms
partitioned/readMetadataLatestPredicate/serial	✅ 1.02x faster	57.3±2.87ms	56.1±2.82ms
v1Checkpoint/readMetadataLatest/serial	✅ 1.01x faster	12.6±0.10ms	12.5±0.31ms
v1Checkpoint/snapshotLatest	☑️ 1.01x slower	782.1±18.32µs	792.4±18.74µs
v2Checkpoint/readMetadataLatest/parallel2	✅ 1.00x	9.9±0.49ms	9.9±0.47ms
v2Checkpoint/readMetadataLatest/serial	✅ 1.00x	14.5±0.11ms	14.5±0.11ms
v2Checkpoint/snapshotLatest	✅ 1.00x	776.5±14.88µs	779.6±24.70µs
wideSchemaJsonStats/readMetadataLatestPredicate/serial	✅ 1.02x faster	77.7±0.77ms	76.0±0.70ms
wideSchemaJsonStats/snapshotLatest	✅ 1.00x	2.4±0.02ms	2.4±0.02ms
wideSchemaStructStats/readMetadataLatestPredicate/serial	✅ 1.00x	34.3±0.44ms	34.3±1.04ms
wideSchemaStructStats/snapshotLatest	✅ 1.00x	2.3±0.02ms	2.3±0.08ms

_{Legend: 🚀 ≥1.15x faster  · ✅ faster or unchanged  · ☑️ ≤1.03x slower  · 🚧 1.03x-1.15x slower  · ❌ ≥1.15x slower
Commit: c0a7a80 · Trigger: auto-push · Tags: base · Updated: 2026-09-17 23:26 PDT}

[github-actions]

AI Review _{(draft - human review required)}

Show review

This PR is described as a mechanical 98% rename, but the parser was reimplemented (Arrow's string_to_datetime/Date32Type::parse replaced by hand-written chrono parsing plus a new parse_partition_date), chrono-tz became a mandatory dependency, and the FFI visitor now threads timestamp_timezone instead of routing configured options to visit_unknown. The timezone-resolution logic and its tests are solid, and the datafusion executor UDF fallback is well shaped. There is one build/test breakage to fix before merge.

Summary

No Delta protocol violation and no correctness defect in the timezone parsing. The one blocking item is the feature-gating regression: the ungated module plus mandatory chrono-tz breaks kernel builds and tests without arrow-expression. The date-path behavior and doc mismatch, two coverage gaps, the FFI ABI shape, and a small doc omission are worth addressing but do not block. The KernelMapToStructUdf Hash/Eq and truncate_subseconds concerns raised during review were checked and found not to be defects.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

This refactor moves partition timestamp parsing into a Kernel-owned chrono implementation and routes both the Arrow evaluator and the scan path through one parse_partition_scalar. The protocol reviewer confirmed the scan-path reroute preserves partition-value semantics for spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ values and empty-string handling. The feature-gating concern from the earlier review was re-checked against this head SHA and found resolved: the ungated module is used by the scan path, the option-dependent items stay gated, and test targets always build with arrow-expression via the dev-dependency graph. Two non-blocking notes follow.

Nit2

kernel/src/timestamp_timezone.rs:1 documents the module as "Reader-timezone handling for TIMESTAMP partition values," but the module now also owns parse_partition_scalar (the shared entry point used by scan/transform_spec.rs for all primitive partition values) and parse_partition_date. Those DATE and TIMESTAMP_NTZ paths are timezone-independent, so a reader looking for general partition-value or date parsing would not expect it here.

Raised by: docs-reviewer, architecture-reviewer

Suggested fix: broaden the module summary, for example "Engine-independent parsing of primitive partition values, including reader-timezone handling for TIMESTAMP."

Summary

No blocking issues. The parsing logic and its timezone and DST handling are correct for spec-compliant values, and the scan-path unification does not change partition-value semantics. The two notes concern documentation accuracy and the undocumented FFI ABI change; neither blocks merge. Candidate test-coverage gaps for the DST fold, gap, and fixed-offset paths were checked and found already covered by existing evaluator tests, and the DATE doc and # Errors wording issues from the prior review remain valid but are unchanged at this SHA.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

This refactor moves partition timestamp parsing into the Kernel-owned timestamp_timezone module and routes both the Arrow evaluator and the scan path through one parse_partition_scalar. The protocol reviewer confirmed spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ values, empty-string handling, the UTC-only rule for TIMESTAMP_NTZ, and reader-timezone-only-on-TIMESTAMP semantics are all preserved, and the DST fold/gap, fixed-offset, and sub-second-truncation paths are correct and directly tested. The feature-gating regression, the # Errors wording, and the coverage gaps from earlier reviews are resolved at this head SHA. No blocking issues.

Summary

No blocking issues and no protocol violation. The chrono parser preserves accepted formats (a superset, no narrowing), empty-string handling, and reader-timezone semantics, and MapToStruct results are unchanged with configured options now routing to the kernel-backed UDF. The two notes concern module placement and an intentional-but-undocumented widening of scan-path acceptance; neither blocks merge. The FFI ABI break (visit_map_to_struct signature change) and the parse_partition_date "preserves Arrow's date forms" doc-parity issue are real but unchanged duplicates of prior AI review findings at this head SHA, so they are not re-raised here. All six primary reviewers completed; no reduced coverage.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

This refactor moves partition-value parsing into a Kernel-owned partition_values.rs, replaces Arrow's string_to_datetime/Date32Type::parse with a hand-written chrono parser, routes both the Arrow evaluator and the scan path through parse_partition_scalar, makes chrono-tz a mandatory dependency, and threads timestamp_timezone through the FFI visitor and a new datafusion KernelMapToStructUdf. The protocol reviewer confirmed empty-string-to-null, DATE/TIMESTAMP/TIMESTAMP_NTZ serialization, and the reader-timezone-only-on-TIMESTAMP rule are all preserved for spec-compliant values. The prior feature-gating blocker and the module-placement concern are resolved at this head SHA. No blocking issues.

Summary

No blocking issues and no Delta protocol violation. Spec-compliant partition values parse identically before and after the refactor; the only behavior change is a broadening of accepted malformed inputs on the scan path, which violates no spec requirement. The two notes concern a doc-grammar slip and an untested cross-path invariant, and neither blocks merge. The FFI ABI break on visit_map_to_struct, the parse_partition_date "preserves Arrow's date forms" doc-parity wording, and the scan-path fail-loud widening are real but are unchanged duplicates of prior AI reviews at this head SHA, so they are not re-raised here; the FFI break in particular is still worth calling out in the changelog against the refactor! marker. All six primary reviewers completed; no reduced coverage.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor moves partition timestamp parsing into a Kernel-owned chrono implementation in kernel/src/partition_values.rs, routes both the Arrow evaluator and the scan-path parse_partition_value_raw through one parse_partition_scalar, makes chrono-tz a mandatory dependency, threads timestamp_timezone through the FFI visitor, and adds a datafusion KernelMapToStructUdf for configured options. Reviewers verified that spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ partition values parse identically before and after, that empty-string-to-null is preserved, and that the reader timezone applies only to TIMESTAMP. The hand-written date and timestamp math was checked for panics and overflow: every year/day/micros conversion is fenced by i32::try_from(...).ok() or parse().ok(), negative-year arithmetic uses div_euclid/rem_euclid correctly, and DST fold/gap and fixed-offset resolution are guarded. Test coverage is good, including the rerouted scan-path TIMESTAMP_NTZ branch and the DST paths via the evaluator tests. The scan-path widening to accept some formerly-rejected malformed inputs, the FFI ABI change to visit_map_to_struct, the parse_partition_date "Arrow forms" doc wording, and the default-vs-kernel MapToStruct divergence are all real but are unchanged duplicates of prior AI reviews at this head SHA, so they are not re-raised; the FFI break is still worth a changelog note against the refactor! marker. All six primary reviewers completed, so coverage is not reduced.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues. Spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ partition values parse identically before and after this refactor, empty-string-to-null is preserved, the reader timezone applies only to TIMESTAMP, and the NTZ UTC-only rule is intact. The hand-written chrono parser was checked for panics and overflow; in particular the truncate_subseconds byte slicing was raised as a possible non-char-boundary panic and disproved, since find('.') returns an ASCII boundary and the truncation branch only runs when at least ten ASCII digit bytes follow the dot, so both slice endpoints are always valid boundaries.

Summary

This is a clean, well-tested refactor that removes a real parsing inconsistency by routing scan-path TIMESTAMP through the same grammar as MapToStruct, and the datafusion KernelMapToStructUdf follows the existing ParseJsonUdf pattern. No protocol violation is present; the only behavior change is a benign broadening of accepted malformed inputs on the scan path. The FFI ABI change to visit_map_to_struct, the module-doc wording, and the default-vs-kernel MapToStruct divergence were reviewed and are unchanged duplicates of prior AI reviews at this head SHA, so they are not re-raised, though the FFI break still warrants a changelog note against the refactor! marker. All six primary reviewers and the disprove gate completed; coverage is not reduced.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor moves reader-timezone parsing for TIMESTAMP partition values into an engine-independent module (kernel/src/partition_values.rs) and routes both the Arrow evaluator and the scan path through the same parse_timestamp seam. Reviewers confirmed that spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ partition values parse identically before and after, empty-string-to-null is preserved, the TIMESTAMP_NTZ UTC-only rule is intact, and the reader timezone applies only to TIMESTAMP. The hand-written chrono parser was checked for panics and overflow: truncate_subseconds slices only at ASCII ./digit boundaries, fixed-offset parsing is range-checked, and the DST fold/gap paths are guarded. The FFI options handle is transferred to the callback exactly once and freed once, matching the documented ownership contract.

Summary

This is a clean, well-tested refactor. The engine-independent TimestampTimezone::parse_timestamp seam is the right cut, and threading MapToStructOptions through the FFI visitor replaces the old visit_unknown escape with a real contract. The one candidate blocker (an FFI double-free on the options handle) was disproved: the callback owns the handle by contract and frees it exactly once. The scan-path widening of accepted malformed inputs, the visit_map_to_struct FFI ABI break and its changelog note, the always-on chrono-tz footprint, and the scan-vs-evaluator DATE/NTZ dispatcher duplication are all real but are unchanged duplicates of prior AI reviews at this head SHA, so they are not re-raised; the FFI ABI break is still worth a changelog line against the refactor! marker. All six primary reviewers and the disprove gate completed; coverage is not reduced.

---

_{Automated review - workflow run}

[dengsh12]

Looks good! Several comments

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor moves reader-timezone TIMESTAMP partition parsing out of the Arrow evaluator into an engine-independent kernel module (kernel/src/partition_values.rs), makes chrono-tz a core dependency, routes both the Arrow evaluator and the scan path through the shared parser, and replaces the FFI visit_unknown escape hatch for configured MapToStruct with a real borrowed-options ABI plus a datafusion KernelMapToStructUdf. Reviewers confirmed the parser is panic- and overflow-safe (truncate_subseconds slices only on ASCII . and digit boundaries, fixed-offset parsing is range-checked, and the DST fold/gap paths are guarded), and that the new unsafe extern "C" visitor honors its documented pointer contract with UTF-8 failures returning 0.

A candidate FFI blocker (unconditional deref of the options pointer in visit_expression_map_to_struct being UB on a null argument) was raised and then dropped: the function's Safety contract explicitly requires options to reference a valid FfiMapToStructOptions, which excludes null, matching the established convention for other unsafe extern "C" entry points in this crate that deref &* pointers under a documented precondition.

Summary

This is a clean, well-tested refactor. Spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ partition values parse identically before and after, empty-string-to-null is preserved, the reader timezone applies only to TIMESTAMP, and the NTZ UTC-only rule is intact; the only behavior change is a benign broadening of accepted malformed inputs. The one candidate blocker was disproved. Prior-review duplicates at this head SHA (the scan-vs-evaluator DATE/NTZ dispatch duplication, the default-vs-kernel MapToStruct divergence, the always-on chrono-tz footprint, and the missing changelog line for the visit_map_to_struct FFI ABI break) are not re-raised, though the ABI-break changelog note against the refactor! marker is still worth adding before merge. All six primary reviewers and the disprove gate completed; coverage is not reduced.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor lifts reader-timezone TIMESTAMP partition parsing out of the Arrow evaluator into an engine-independent kernel/src/partition_values.rs, makes chrono-tz a mandatory kernel dependency, routes both the Arrow evaluator and the scan-path parse_partition_value_raw through the shared TimestampTimezone::parse_timestamp seam, and replaces the old visit_unknown MapToStruct escape with a real borrowed-options FFI ABI plus a datafusion KernelMapToStructUdf. Reviewers confirmed spec-compliant DATE, TIMESTAMP, and TIMESTAMP_NTZ partition values parse to identical instants before and after, empty-string-to-null is preserved, the TIMESTAMP_NTZ UTC-only rule is intact, and the reader timezone applies only to offset-less TIMESTAMP. The hand-written chrono parser was checked for panics and overflow: truncate_subseconds slices only on ASCII ./digit boundaries, fixed-offset parsing is range-checked, and the DST fold/gap paths are guarded. The FFI options handle is borrowed for the call/callback and freed once in the C example.

Summary

Clean, well-tested refactor. The engine-independent parse seam is the right cut, spec-compliant partition values are unchanged, and the FFI options ABI is a real improvement over the prior sentinel. The one candidate FFI null-deref blocker is an unchanged same-SHA duplicate that was already disproved and is confirmed consistent with the crate's extern-pointer convention, so it is not re-raised; the scan-path malformed-input broadening, the divergences-list wording, the always-on chrono-tz footprint, the DATE/NTZ dispatch fork, and the missing refactor! changelog line for the visit_map_to_struct ABI break are also unchanged duplicates and stay suppressed, though the changelog note is still worth adding before merge. All six primary reviewers completed and no disprove gate was required since nothing blocking was published; coverage is not reduced.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor lifts TIMESTAMP partition parsing into an engine-independent kernel/src/partition_values.rs, makes chrono-tz a mandatory kernel dependency, routes both the Arrow evaluator and the scan-path parse_partition_value_raw through the shared TimestampTimezone::parse_timestamp seam, and replaces the FFI visit_unknown MapToStruct escape with a borrowed-options ABI plus a datafusion KernelMapToStructUdf. The protocol reviewer confirmed both spec-compliant TIMESTAMP encodings parse to identical instants before and after on both paths, empty-string-to-null is preserved, TIMESTAMP_NTZ stays UTC-only, the 18h offset bound is consistent, and DST fold/gap resolution matches Spark's java.time semantics. The hand-written parser was checked for panics and overflow. The narrowing of accepted malformed inputs, the missing changelog line for the visit_map_to_struct ABI break, the always-on chrono-tz footprint, and the module-home/DATE-NTZ dispatch fork are unchanged duplicates of prior reviews at this head SHA and are not re-raised.

Summary

Clean, well-tested refactor with the parse seam in the right place and read semantics preserved for both spec-compliant TIMESTAMP encodings. The only new finding is a factually inverted line in the KNOWN DIVERGENCES doc comment, which is non-blocking. All six primary reviewers completed and coverage is not reduced; no disprove gate was required since nothing blocking was published.

---

_{Automated review - workflow run}

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor lifts TIMESTAMP partition parsing into an engine-independent kernel/src/partition_values.rs and routes both the Arrow evaluator and the scan path through one TimestampTimezone::parse_timestamp seam. The protocol reviewer confirmed all three spec-defined TIMESTAMP encodings parse to identical instants on both paths, non-spec forms are rejected, and empty-string-to-null, the TIMESTAMP_NTZ no-offset rule, offset precedence, DST fold/gap resolution, and the 18h offset bound all hold. The maintainer passes verified the hand-written parser is panic- and overflow-safe (bounds-checked byte access, char-safe slicing, checked_sub_signed in the gap walk-back) and that the arrow-expression gating and the now-mandatory chrono-tz dependency are consistent. The previously flagged inverted KNOWN DIVERGENCES doc line and the missing scan-path malformed-TIMESTAMP test are both resolved in this diff.

The two notes below are small test-coverage gaps; neither blocks merge.

Summary

Clean, well-scoped refactor with the parse seam in the right place; the shared parser is protocol-compliant and safe, and scan/evaluator paths agree on spec-compliant values. The only additions worth making are two small negative/boundary test cases. All six primary reviewers completed, so coverage is not reduced; no disprove gate was required since nothing blocking was published.

---

_{Automated review - workflow run}

[github-actions]

Nit1 The || raw.ends_with('z') guard in parse_explicit_offset_timestamp has no dedicated test. Existing reject cases do not reach it: lowercase_t fails the byte-10 T check first, and invalid_suffix/trailing_garbage_after_zone end in uppercase, so parse_from_rfc3339 rejects them regardless of the guard. Deleting or inverting the guard would leave every test green while chrono silently accepts lowercase z, diverging from the protocol's uppercase-Z form.

Raised by: test-coverage-reviewer.

Suggested fix: add a reject case exercising the guard, e.g. #[case::lowercase_z("2024-01-15T12:30:45z")].

[github-actions]

Nit2 The 18h embedded-offset bound (local_minus_utc().unsigned_abs() <= 18 * 3_600) is tested only on the reject side (offset_past_limit = +19:00). The +18:00/-18:00 accept cases test the configured Fixed reader offset, a different code path, so the <= comparison on the embedded-offset path is not pinned against an off-by-one regression.

Raised by: test-coverage-reviewer.

Suggested fix: add an accept case at the boundary, e.g. #[case::max_offset("2024-01-15T12:30:45+18:00", "2024-01-14T18:30:45Z")], and a reject case #[case::offset_one_past_limit("2024-01-15T12:30:45+18:01")].

[github-actions]

AI Review _{(draft - human review required)}

Show review

No blocking issues.

This refactor lifts TIMESTAMP partition parsing into an engine-independent kernel/src/partition_values.rs and routes both the Arrow evaluator (parse_partition_scalar) and the scan path (parse_partition_value_raw) through one TimestampTimezone::parse_timestamp seam. The protocol reviewer confirmed all three spec-defined TIMESTAMP encodings parse to identical instants on both paths, explicit-offset precedence holds, empty-string-to-null and the TIMESTAMP_NTZ UTC-only rule are preserved, DST fold/gap resolution matches java.time semantics, the 18h offset bound is consistent, and the narrowing of accepted inputs removes only non-spec forms. Both maintainer passes verified the hand-written parser is panic- and overflow-safe (bounds-checked get(10), char-safe ends_with/split_once, guarded parse_two_digits, checked_sub_signed in the gap walk-back) and that making chrono-tz mandatory while keeping try_from_options/MapToStructOptions under #[cfg(feature = "arrow-expression")] stays coherent for no-default and all-features builds. Docs were confirmed accurate, including the corrected KNOWN DIVERGENCES direction.

Summary

Clean, well-scoped refactor with the parse seam cut at the right layer boundary; the shared parser is protocol-compliant and safe, and the scan and evaluator paths agree on spec-compliant values. All six primary reviewers completed, so coverage is not reduced. The only new item is a minor test-assertion tightening; the two previously reported test nits are duplicates at this unchanged head SHA and are not re-raised.

---

_{Automated review - workflow run}

[github-actions]

Nit1 In test_parse_partition_value_raw_protocol_timestamp, the malformed-TIMESTAMP and DATE/NTZ rejection cases assert only .is_err(). A regression that returned a different Err variant, or a DATE/NTZ value that started succeeding through a different match arm, would still pass, so the assertions do not pin the intended Error::ParseError failure mode.

Raised by: test-coverage-reviewer.

Suggested fix: match on the error variant for the rejection cases, e.g. assert matches!(err, Error::ParseError(..)) instead of .is_err().