feat!: add reader timezone to map-to-struct - #3118
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
+ Coverage 90.32% 90.37% +0.04%
==========================================
Files 250 251 +1
Lines 89038 89432 +394
Branches 89038 89432 +394
==========================================
+ Hits 80422 80821 +399
+ Misses 5682 5669 -13
- Partials 2934 2942 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
breaking-change
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 13 · ☑️ 2 · 🚧 0 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
DrakeLin
changed the title
1f98a98 to
2c0eda8
Compare
DrakeLin
changed the title
2c0eda8 to
3529fd1
Compare
9580a36 to
907de2e
Compare
907de2e to
e253412
Compare
kyli87
left a comment
kyli87
left a comment
There was a problem hiding this comment.
Left some questions/comments. I think I'm not fully grasping how this will be used to solve the ambiguous timezone problem.
My impression is that when kernel generates a MapToStruct expression (I assume these only ever come from kernel), then it will append some timezone info. Will it just take the current machine's timezone or will the timezone come from some serialized location somehow...?
375e773 to
14e296d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads a connector-supplied reader timezone through MapToStruct for offset-less TIMESTAMP partition values, defaulting to UTC. The core semantics are sound: DST overlap resolves to the earliest instant, forward gaps shift by the gap size, embedded offsets and named zones take precedence, fixed-offset validation enforces the +/-18:00 bound, and TIMESTAMP_NTZ stays timezone-independent. The fail-closed design is the right call: configured options use a distinct proto tag (14) so legacy decoders reject them, and both the DataFusion executor and the FFI visitor refuse configured options rather than silently applying UTC. Test coverage is thorough and the docs match the implementation. No blocking issues.
Non-blocking notes
Nit1: kernel/src/timestamp_timezone.rs:110, parse_timestamp_with_timezone. The earliest-occurrence (overlap) and shift-forward (gap) semantics only hold because the fast path string_to_datetime(&timezone, raw) errors on ambiguous and skipped local times, letting the resolve_local_timestamp fallback own those cases. This is an implicit dependency on arrow's current behavior with no compile-time signal if a future arrow version resolves ambiguity itself. Raised by: maintainer-claude-reviewer. Suggested fix: add a short comment at the fast-path call site noting the fallback owns overlap/gap semantics and that the existing dst_overlap/dst_gap tests guard that external invariant.
Nit2: kernel/src/lib.rs:119, new module timestamp_timezone. The module depends entirely on arrow and has a single consumer in engine/arrow_expression, yet it sits at the crate root with a #[cfg(all(feature = "arrow-expression", feature = "default-engine-base"))] gate that duplicates the gate already on engine::arrow_expression. Sibling arrow-dependent modules live under engine/. Raised by: architecture-reviewer. Suggested fix: move it to kernel/src/engine/arrow_expression/timestamp_timezone.rs, which inherits the parent module's cfg and drops the duplicated attribute.
Nit3: kernel/src/plans/proto/convert.rs:326. The "is this expression non-default?" predicate is open-coded as options != MapToStructOptions::default() here and at two other sites (datafusion-executor/src/expression.rs, ffi/src/expressions/engine_visitor.rs), while MapToStructOptions::is_default already exists for serde. The fail-closed guarantee lives in exactly these branches, so keeping them in sync matters. Raised by: architecture-reviewer. Suggested fix: expose is_default as pub(crate) and use it at all three call sites.
Nit4: kernel/src/timestamp_timezone.rs:43, parse_timestamp doc. The comment scopes the overlap/gap rules to "named reader timezones," but resolve_local_timestamp governs embedded named zones too (exercised by embedded_iana_dst_overlap/embedded_iana_dst_gap). Raised by: docs-reviewer. Suggested fix: broaden the wording to "named timezones, whether the reader timezone or one embedded in the value."
Summary
The feature is well-scoped and the fail-closed proto/adapter design correctly prevents older executors from silently reinterpreting configured timestamps as UTC. Reviewers confirmed the DST, embedded-zone, and fixed-offset semantics against tests and delta-spark's session-timezone behavior; the reader-vs-writer timezone gap is an inherent protocol limitation that Kernel cannot resolve and matches the reference implementation. The remaining items are documentation and maintainability polish.
Automated review - workflow run
1dcd079 to
8a7ca6e
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The design is sound: default semantics stay on the legacy proto tag 12 while configured options go on a distinct tag 14, so older executors fail closed instead of silently applying UTC, and the DataFusion and FFI adapters reject configured options rather than lowering them with default meaning. The timezone module handles the subtle cases correctly, with DST overlap resolving to the earlier instant, the forward-gap shift, embedded offset and named-zone precedence, and the fixed-offset bounds all matching the spec and Spark atZone behavior, and the tests assert exact instants including the Pacific/Apia full-day skip. TIMESTAMP_NTZ stays timezone-independent.
Non-blocking notes
Nit1. kernel/src/expressions/mod.rs (serde on MapToStructExpression::options). The proto path fails closed for configured options via the distinct tag 14, proven by configured_map_to_struct_is_unknown_to_legacy_proto. The serde JSON path does not have an equivalent guard: options uses skip_serializing_if with no deny_unknown_fields, so if serde JSON is ever a cross-version boundary, an older kernel would silently ignore the options key and parse as UTC, producing a wrong instant instead of a rejection. This is a human-judgment call and depends on whether expression JSON crosses versions.
Raised by: maintainer-claude-reviewer.
Suggested fix: if serde JSON can cross kernel versions, mirror the proto fail-closed shape (a distinct configured variant); if it is same-version only, add a short comment saying so.
Nit2. kernel/src/checkpoint/checkpoint_transform.rs (build_partition_values_parsed_expr). Checkpoint reconstruction hardcodes MapToStructOptions::default() (UTC), and scan/log-replay prefer the native partitionValues_parsed column over string reconstruction. This is not a regression here since every caller in this PR uses UTC, but once the later stacked PR applies a non-UTC reader timezone to scans, UTC-baked offset-less timestamp partition values in a kernel-written checkpoint would be trusted over the correctly adjusted reconstruction.
Raised by: delta-protocol-reviewer.
Suggested fix: track this as a prerequisite for the timezone-wiring PR (propagate timezone into checkpoint reconstruction, or do not prefer the native parsed column for offset-less timestamp partitions) and note the coupling in the deterministic-UTC comment.
Nit3. datafusion-executor/src/expression.rs and ffi/src/expressions/engine_visitor.rs use is_default() as the capability gate. is_default() answers "is this the legacy wire representation" for the proto tag decision, which is correct, but the two adapters reuse it to mean "can this adapter execute these options". Those questions coincide only while timezone is the sole option; a future option a given adapter could honor would still be rejected by !is_default(), and the coupling is invisible to the compiler.
Raised by: architecture-reviewer.
Suggested fix: keep is_default() for the wire-tag decision and have adapters gate on the specific unsupported capability (timestamp_timezone().is_some() today) so the check is expressed against the feature.
Automated review - workflow run
sanujbasu
left a comment
sanujbasu
left a comment
There was a problem hiding this comment.
I took another pass over the current HEAD. Most of these are API and documentation cleanup; the trailing named-timezone behavior is the one code-path question I think we should settle before merge.
8a7ca6e to
e8de20d
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The timezone semantics (earliest-instant on a backward DST overlap, forward shift on a spring-forward gap, the 48-hour walk-back that also covers full-day transitions like Pacific/Apia, the +/-18:00 fixed-offset range, embedded-offset precedence, and TIMESTAMP_NTZ independence) are correct and match delta-spark's session-timezone behavior. The fail-closed wire design is sound: a distinct proto tag (14) and skip_serializing_if keep the default form byte-compatible while forcing older executors to reject configured options instead of silently applying UTC, and both the DataFusion and FFI adapters reject configured options rather than reinterpreting them. Test coverage is strong and value-specific across parsing, serde, proto routing, transforms, and adapter behavior. Within this PR every production call site uses default UTC options, so there is no present pruning-vs-materialization divergence.
Non-blocking notes
Nit1: ffi/src/expressions/engine_visitor.rs:700. A configured (unsupported) MapToStruct is reported to visit_unknown with the name "map_to_struct", the same label a supported form would carry, so an engine cannot distinguish a fail-closed rejection from a genuinely opaque expression in its logs. Raised by: maintainer-claude-reviewer, disprove-reviewer (NITPICK). Suggested fix: report a distinct name such as "configured_map_to_struct", matching the proto tag naming.
Nit2: kernel/src/engine/arrow_expression/timestamp_timezone.rs:17. The offset validation (+/-18:00 range) and DST resolution policy are engine-neutral protocol semantics, but they live in a pub(crate) type inside the arrow evaluator. The PR description says the next stacked PR adds configured execution to the DataFusion and FFI adapters, which cannot reach this type and would re-derive the same policy, risking divergence between engines. Raised by: architecture-reviewer, disprove-reviewer (NITPICK). Note: relocating next to PrimitiveType::parse_scalar is not a drop-in move because that parser is arrow-independent while this logic depends on arrow's Tz/string_to_datetime. Suggested fix: before the next stacked PR lands, extract the validation and resolution policy into a shared, engine-neutral (or deliberately feature-gated) utility both adapters can call, so the +/-18:00 rule and overlap/gap policy are defined once.
Nit3: kernel/src/engine/arrow_expression/timestamp_timezone.rs:143. Two internal fall-through branches in split_embedded_named_timezone are untested: the timestamp.get("YYYY-MM-DD".len()..) guard for short or multibyte prefixes, and a trailing token that is alphabetic but not a parseable Tz. Both degrade to a clean parse error today, but a future refactor to byte indexing (timestamp[10..]) could turn the guard into a panic, and the alphabetic-but-invalid path is only exercised as a configured value, not as an embedded token. Raised by: test-coverage-reviewer, maintainer-codex-reviewer. Suggested fix: add cases for a short/multibyte prefix followed by a valid IANA token (assert a clean error, not a panic) and for an embedded token like "2024-01-15 12:30:45 Foo/Bar" (assert it is not misreported as an offset/zone conflict); optionally add +18:30 to the invalid-offset cases.
Summary
This is a careful, well-tested change with no blocking issues. The timestamp parsing semantics, fail-closed wire format, and adapter rejection behavior are correct and thoroughly covered. The remaining items are advisory: an FFI diagnostic label, where the reusable timezone policy should live before the next stacked PR needs it, and a few internal fall-through branches worth pinning with tests. The protocol reviewer confirmed zero spec violations in this PR; the checkpoint/data-skipping/reader-timezone consistency concern is latent and only becomes actionable when a later stacked PR wires a non-UTC reader timezone into materialization.
Automated review - workflow run
e8de20d to
4e74a27
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change adds a connector-supplied reader timezone to MapToStruct and implements offset-less TIMESTAMP partition-value parsing in a focused timestamp_timezone module. The core semantics are correct: DST fall-back overlap uses the earlier occurrence, spring-forward gaps shift forward by the transition offset (verified against the Pacific/Apia whole-day skip), fixed offsets are normalized within +/-18:00, an embedded offset or named zone takes precedence over the reader timezone, TIMESTAMP_NTZ stays timezone-independent, and the dual proto tag makes older executors fail closed rather than silently applying UTC. One candidate blocker on the DST-gap backtracking loop was examined and disproved: applying the pre-transition offset is algebraically equivalent to the forward-shift semantics, and no failing tzdb input was demonstrated. No blocking issues.
Non-blocking notes
Nit1: datafusion-executor/src/expression.rs (can_lower_map_to_struct_options) and ffi/src/expressions/engine_visitor.rs (can_visit_map_to_struct) both gate executor capability on options.is_default(). That predicate answers "are any options set", not "can this executor run these options". The two coincide today because there is exactly one option, but when configured execution lands in the next stacked PR these gates must become a per-field check, and adding a second option would make is_default() reject expressions an executor could actually run. The proto converter's use of is_default() for tag selection is correct and should stay. Raised by: architecture-reviewer. Suggested fix: at the executor gates, inspect the specific field (options.timestamp_timezone().is_none() today) behind a helper named for the capability, leaving is_default() for the wire-tag decision.
Nit2: kernel/src/engine/arrow_expression/evaluate_expression.rs parse_partition_scalar now parses TIMESTAMP through the new parse_timestamp while TIMESTAMP_NTZ still uses arrow's string_to_datetime. The two sibling types must accept the same wall-clock grammar, but that grammar now has two independent implementations that can drift; the new parser also narrows the accepted date form (it requires hyphenated %Y-%m-%d and no longer accepts compact %Y%m%d) relative to the arrow path NTZ keeps. Spec-compliant writers are unaffected. Raised by: architecture-reviewer, maintainer-claude-reviewer. Suggested fix: have the new module own local-timestamp parsing for both arms (or have NTZ delegate to the same primitive) so the format set is enforced in one place, and note the intentional narrowing.
Nit3: kernel/src/lib.rs declares timestamp_timezone at the crate root, but it is pub(crate), gated on arrow-expression, and its only consumer is the arrow evaluator; sibling evaluator internals live under engine/arrow_expression/. Raised by: architecture-reviewer. Suggested fix: move it to engine/arrow_expression/timestamp_timezone.rs so the placement matches its sole consumer and the feature gate stays local to that subtree.
Nit4: kernel/src/timestamp_timezone.rs resolve_local_timestamp uses a (1..=48) backtracking window without explaining the bound. Raised by: maintainer-claude-reviewer. Suggested fix: add a one-line comment noting the largest real transition gap is 24h (Samoa 2011) plus up to ~24h to reach a valid wall-clock instant, so 48 is safe.
Nit5: kernel/src/expressions/mod.rs with_timestamp_timezone documents precedence but not that a value carrying both a named zone and a numeric offset is rejected (surfaced as ParseError). This is the public contract for connector authors. Raised by: maintainer-claude-reviewer, delta-protocol-reviewer. Suggested fix: add a clause stating that a value with both an embedded zone and an offset is an error rather than a precedence resolution.
Nit6: kernel/src/timestamp_timezone.rs parse_partition_fixed_offset (the parser for offsets embedded in values, distinct from the well-tested reader-tz parser) leaves three branches untested: the negative-sign arm, the compact 6-digit +HHMMSS form, and the reject arms (extra colon component, bad compact length). Raised by: test-coverage-reviewer. Suggested fix: add a small rstest through parse_timestamp (default UTC), e.g. "...45-05:30", "...45+053015", and malformed "...45+05:30:15:00" / "...45+053", asserting resolved micros or None.
Summary
The timezone semantics are implemented correctly and match both PROTOCOL.md and Spark's local-resolution behavior, the wire compatibility strategy is fail-closed and tested, and coverage of the new logic is strong. The notes above are maintainability and documentation refinements, not defects. No blocking issues.
Automated review - workflow run
4e74a27 to
56d9811
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR threads a connector-supplied reader timezone through MapToStruct so offset-less TIMESTAMP partition values can be interpreted in an IANA zone or fixed offset instead of always UTC. I reviewed protocol semantics, Rust correctness of the new timestamp_timezone module, the API/serde/proto surface, tests, and docs.
No blocking issues. The DST overlap/gap resolution, embedded-offset precedence, both-present rejection, TIMESTAMP_NTZ/DATE independence, and serde/proto default-elision are all correct and well tested. One candidate blocker about split_fixed_offset misparsing negative-year dates was checked and dropped: the offset scan only runs after the first T/t/space separator, so date hyphens are never treated as offsets.
Non-blocking notes
Nit1: ffi/src/expressions/engine_visitor.rs (configured MapToStruct arm, around line 700). A MapToStruct carrying configured options is reported through visit_unknown and its child map expression is intentionally not visited. FFI expression walkers that extract column dependencies therefore lose column lineage for configured expressions. This is documented and intentional for this PR (configured execution is deferred to a later stacked PR), so it is not a defect, but it is worth resolving before configured expressions reach FFI consumers. Raised by: maintainer-codex-reviewer. Suggested fix: add a distinct callback that carries the options plus the child list so walkers can still see the referenced columns, rather than collapsing to visit_unknown.
Nit2: datafusion-executor/src/expression.rs can_lower_map_to_struct_options (around line 451) and ffi/src/expressions/engine_visitor.rs can_visit_map_to_struct (around line 717). Both adapter guards test options.timestamp_timezone().is_none(), while the serde and proto paths use the canonical MapToStructOptions::is_default(). With a single option field today these are equivalent, so there is no current bug. When a second option field is added, these guards would under-reject: an expression carrying the new field but no timezone would be lowered/visited as default and silently drop the option, while proto would correctly treat it as configured. Raised by: maintainer-claude-reviewer, architecture-reviewer. Suggested fix: have both guards call map_to_struct.options.is_default().
Summary
The change is careful and correct within its stated UTC-only-execution scope, with strong test coverage of the timezone parsing and DST edge cases and clean wire backward compatibility. The two notes are latent-trap and future-proofing items, not present defects. The reader-timezone-for-writer-timezone substitution is a deliberate, documented protocol interpretation that matches Spark and is the only tractable option since the writer timezone is not recorded in the log.
Automated review - workflow run
56d9811 to
5c957bd
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. This change is well scoped and thoroughly tested. The new timestamp_timezone module handles the hard cases (DST overlap and gap, second-precision fixed offsets, fractional truncation, embedded offset vs named zone precedence), and the serde/proto changes keep the wire format backward compatible (default options are omitted and the existing oneof tag is preserved). Every production call site in this PR still passes MapToStructOptions::default() (UTC), so live scan behavior is unchanged here.
Two candidate defects raised during review were checked and dropped: the "malformed mixed offset plus named zone is silently accepted" claim (that exact input is rejected by parse_local_timestamp), and the "TIMESTAMP vs TIMESTAMP_NTZ parser asymmetry" claim (the two types have intentionally different, documented accepted formats and the noncanonical inputs cited are not promised to be accepted equally).
Non-blocking notes
Nit1. datafusion-executor/src/expression.rs and ffi/src/expressions/engine_visitor.rs gate on options.timestamp_timezone().is_none() rather than the existing MapToStructOptions::is_default(). Today these are equivalent, so there is no current defect. The maintenance cost is that adding a second field to MapToStructOptions would make both adapters silently accept a configured expression they cannot execute, with no compile error to catch it.
Raised by: architecture-reviewer.
Suggested fix: gate both helpers on !map_to_struct.options.is_default() so growing the options struct forces a deliberate revisit of each adapter.
Nit2. kernel/src/expressions/mod.rs: MapToStructOptions::is_default is pub, but its only callers are the in-crate skip_serializing_if attribute and the proto conversion. with_timestamp_timezone and timestamp_timezone() need to be public for connectors; is_default does not appear to.
Raised by: architecture-reviewer, maintainer-claude-reviewer.
Suggested fix: make is_default pub(crate) unless a downstream consumer needs it.
Nit3. kernel/src/timestamp_timezone.rs: the TimestampTimezone enum doc says it "cannot use Arrow's Tz", but the Named variant wraps chrono_tz::Tz, not an Arrow type. The rationale reads as if it refers to the type the enum actually holds.
Raised by: docs-reviewer.
Suggested fix: phrase the rationale in terms of the type used, e.g. a named-zone type alone cannot represent normalized second-precision fixed offsets, so Fixed carries a FixedOffset.
Nit4. Forward-looking, for the later stacked PRs rather than this one. Checkpoint partitionValues_parsed is precomputed under UTC (see the comment in checkpoint_transform.rs), and the read path prefers that native column over the map_to_struct fallback. When a non-UTC reader timezone is wired into scans, offset-less TIMESTAMP partitions from a checkpoint would keep UTC-parsed instants while the JSON-commit fallback would apply the reader timezone, and data skipping must stay on the same timezone as materialization or pruning could drop matching files.
Raised by: delta-protocol-reviewer.
Suggested fix: track this as a stack invariant; when the connector timezone is enabled, either avoid precomputing zone-dependent typed partition values in checkpoints or re-derive them from the raw map under the active reader timezone, and update data_skipping in lockstep.
Summary
The core timezone semantics are correct and match the delta-spark / delta-kernel-java reference behavior, wire compatibility is handled carefully with regression tests, and coverage of the new logic paths is good. There are no blocking issues; the notes above are maintainability and forward-stack items.
Automated review - workflow run
5c957bd to
cafbb95
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The reader-timezone design is sound: offset-less TIMESTAMP partition values are interpreted in a connector-supplied reader timezone (defaulting to UTC), embedded offsets and named zones take precedence, TIMESTAMP_NTZ stays zone-independent, and DST overlap/gap resolution matches java.time/Spark. UTC-default behavior is preserved end to end across serde, proto (tag 2 added, existing tag preserved), the DataFusion and FFI fail-closed guards, and all internal call sites. Test coverage of the timezone contract is thorough.
Non-blocking notes:
Nit1: kernel/src/engine/arrow_expression/evaluate_expression.rs:966
The default (UTC) TIMESTAMP path no longer routes through arrow's string_to_datetime; it now uses the hand-rolled parse_local_timestamp grammar. This is a read path for partition values in already-written tables, so any zone-less form that string_to_datetime accepted but the new grammar rejects is a silent read regression, not just a leniency change. No reviewer produced a concrete regressing TIMESTAMP input, but the parser swap is real and the accepted-set is now defined by the new code rather than arrow's.
Raised by: maintainer-claude-reviewer, maintainer-codex-reviewer, delta-protocol-reviewer
Suggested fix: confirm the new grammar accepts a superset of the prior default-path inputs, and if any form is intentionally dropped, state that as a deliberate compatibility narrowing in the PR/code rather than relying on the test matrix, which is defined by the new parser's own contract.
Nit2: kernel/src/expressions/mod.rs:665
The reader timezone is modeled as a per-expression option, but the doc itself states it must be identical across all partition-value expressions in one scan. That invariant is a convention four independent construction sites (data_skipping, log_replay, scan_plan, checkpoint_transform) must each uphold; nothing structural enforces it. All sites pass the default today, so there is no present bug, but when the connector timezone is wired in, a missed site makes pruning and materialization disagree on an instant with no type error.
Raised by: architecture-reviewer
Suggested fix: give the timezone a single scan-level home and stamp it onto partition-value expressions through one helper, so the field stays on the expression for transport while the value has one owner.
Nit3: datafusion-executor/src/expression.rs:407
The diff deleted the blank /// line before /// # Errors, so the heading now immediately follows the Decimal bullet. Rustdoc requires a blank line before a heading, so # Errors will render as part of the preceding list item rather than as a section.
Raised by: docs-reviewer
Suggested fix: restore the blank /// line between the Decimal bullet and /// # Errors.
Nit4: kernel/src/transforms/expression.rs:347
The rebuild closure constructs the struct literal by hand (including Box::new) instead of using the existing MapToStructExpression::new constructor, duplicating the boxing at a second site.
Raised by: maintainer-claude-reviewer
Suggested fix: use MapToStructExpression::new(map_expr, expr.options.clone()).
Automated review - workflow run
kyli87
left a comment
kyli87
left a comment
There was a problem hiding this comment.
Code mostly LGTM, but have some questions about where we could possibly simplify. Also some documentation clarifications.
cafbb95 to
b1ba2d8
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The reader-timezone design is correct and matches the delta-spark / delta-kernel-java reference: offset-less TIMESTAMP partition values resolve in the connector-supplied reader timezone (UTC by default), embedded offsets and named zones take precedence via Arrow's string_to_datetime, TIMESTAMP_NTZ and DATE stay timezone-independent, and DST overlap (earlier instant) and gap (prior offset) resolution line up with java.time. The default path still routes through string_to_datetime with a zero-offset zone, so the accepted-format set for already-written tables is unchanged. Serde and proto stay backward compatible (default options omitted, existing tag preserved), and the DataFusion and FFI adapters fail closed on any configured option. One candidate blocker about negative-zero fixed offsets was checked and dropped: only -00:00 maps to zero (semantically identical to +00:00), sub-hour negatives like -00:30 stay negative, and the raw option string is what serializes, so there is no round-trip mismatch.
Summary
The change is well scoped and thoroughly tested, the timezone semantics match the reference implementations, and wire compatibility is preserved. Every production call site still passes MapToStructOptions::default(), so live scan behavior is unchanged by this PR. There are no blocking issues; the notes above are placement, validation-altitude, and coverage-completeness items. All six reviewers and the disprove gate completed.
Automated review - workflow run
b1ba2d8 to
e894a5b
Compare
e894a5b to
8d8ee0b
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The reader-timezone design holds up across every reviewer pass. Offset-less TIMESTAMP partition values resolve in the connector-supplied reader timezone (UTC by default), embedded offsets and named zones take precedence through Arrow's string_to_datetime, TIMESTAMP_NTZ and DATE stay timezone-independent, and the DST overlap (earlier instant) and gap (prior offset) resolution match java.time/Spark. The default path routes through string_to_datetime with the transition-free UTC zone, so the accepted-format set for already-written tables is unchanged. Serde and proto stay backward compatible (default options omitted, existing tag preserved), and the DataFusion and FFI adapters fail closed on any configured option.
One candidate blocker was checked and dropped: a claim that TIMESTAMP_NTZ parsing regresses accepted formats. The TIMESTAMP_NTZ arm still uses string_to_datetime(&Utc, raw) and is not touched by this diff, so any leniency there is pre-existing, not introduced here.
Summary
The change is well scoped and thoroughly tested, and the timezone semantics match the reference implementations while preserving wire compatibility and the default UTC read path. Every production call site still passes MapToStructOptions::default(), so live scan behavior is unchanged by this PR. All six reviewers and the disprove gate completed; the two notes above are minor form and coverage items.
Automated review - workflow run
| Ok(struct_null_when_not(map.is_not_null(), named_struct(args))) | ||
| } | ||
|
|
||
| fn can_lower_map_to_struct_options(map_to_struct: &MapToStructExpression) -> bool { |
There was a problem hiding this comment.
Nit1 can_lower_map_to_struct_options here and can_visit_map_to_struct in ffi/src/expressions/engine_visitor.rs are one-line wrappers whose whole body is map_to_struct.options.is_default(), restating the same "adapter supports only default options" concept under two names in two crates. MapToStructOptions::is_default() is already public for exactly this cross-crate use.
Raised by: architecture-reviewer
Suggested fix: call map_to_struct.options.is_default() directly at each guard site, or express "needs engine support beyond default" as one method on MapToStructExpression that both adapters call.
| if entry_idx >= entry_start && map_values.is_valid(entry_idx as usize) { | ||
| let raw = map_values.value(entry_idx as usize); | ||
| match parse_partition_scalar(target_types[i], raw)? { | ||
| match parse_partition_scalar(target_types[i], raw, timestamp_timezone)? { |
There was a problem hiding this comment.
Nit2 parse_partition_scalar now threads timestamp_timezone, but only the TIMESTAMP arm consumes it; DATE ignores it. TIMESTAMP_NTZ zone-independence is pinned by a test, DATE zone-independence is not. Not a present risk (the DATE arm never receives the zone), just an untested contract.
Raised by: test-coverage-reviewer
Suggested fix: add one rstest case asserting a DATE parsed with a configured reader timezone equals the UTC-default result, reusing the existing evaluate_map_timestamp_timezone harness.
8d8ee0b to
2f4b237
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The reader-timezone handling is correct and matches the java.time/Spark reference: offset-less TIMESTAMP partition values resolve in the connector-supplied reader timezone (UTC by default), embedded offsets and named zones take precedence through Arrow's string_to_datetime, TIMESTAMP_NTZ and DATE stay timezone-independent, and DST overlap (earlier instant) and gap (prior offset) resolution line up with atZone. The default path routes through string_to_datetime with a zero-offset zone, so the accepted-format set for already-written tables is unchanged. Serde and proto stay backward compatible (default options omitted, existing tag preserved), and the DataFusion and FFI adapters fail closed on any configured option. Every production call site still passes MapToStructOptions::default(), so live scan behavior is unchanged by this PR.
One candidate blocker was checked and dropped: a claim that the FFI visitor skips child traversal for a configured MapToStruct and can drop column dependencies. The configured node is intentionally represented as a terminal Unknown (whose callback carries no child-list id), which matches the existing unsupported-Cast handling and is documented and tested. Visiting the child first would create an unattached child list, so this is consistent fail-closed behavior, not a defect.
Summary
The change is well scoped and thoroughly tested. Timezone semantics match the reference implementations, wire compatibility is preserved, and the two engine adapters fail closed on configured options. All six reviewers and the disprove gate completed; the two notes above are minor cleanup items.
Automated review - workflow run
| } | ||
|
|
||
| #[cfg(test)] | ||
| fn expected_timestamp_micros(value: &str) -> i64 { |
There was a problem hiding this comment.
Nit1 This #[cfg(test)] expected_timestamp_micros helper is redefined verbatim in the timestamp_timezone test module. The evaluate_expression tests already import it via super::super::expected_timestamp_micros, so the duplicate is avoidable maintenance surface. Raised by: maintainer-claude-reviewer. Suggested fix: import the shared helper in the timestamp_timezone tests and delete the local copy.
2f4b237 to
8f34a30
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The reader-timezone handling is correct and matches the java.time/Spark reference. Offset-less TIMESTAMP partition values resolve in the connector-supplied reader timezone (UTC by default), embedded offsets and named zones take precedence through Arrow's string_to_datetime, and TIMESTAMP_NTZ and DATE stay timezone-independent. DST overlap resolves to the earlier instant (first.min(second)) and gap resolution applies the prior offset to the original local time, which is algebraically the java.time gap-forward rule and holds for full-day skips such as Pacific/Apia.
One candidate blocker was checked and dropped: a claim that the whole-hour lookback in resolve_nonexistent_local_timestamp picks the post-transition offset for sub-hour DST gaps (for example Australia/Lord_Howe). Subtracting time from a gap-local instant cannot land in the post-transition local interval, so the first one-hour lookback already reaches valid pre-transition time and yields the correct offset; the disprove gate confirmed this is not a present defect.
The default path still routes through string_to_datetime with a zero-offset zone, so the accepted-format set for already-written tables is unchanged. Serde and proto stay backward compatible (default options omitted, existing tags preserved), the proto path is emission-only, expression transforms preserve options via expr.options.clone(), and the DataFusion and FFI adapters fail closed on any configured option. Every production call site still passes MapToStructOptions::default(), so live scan and checkpoint behavior is unchanged.
All six reviewers and the disprove gate completed. Remaining reviewer notes were minor and either already raised in prior review rounds (duplicate is_default() guard wrappers, validation altitude of with_timestamp_timezone, parse_digits fixed-width params, DATE zone-independence not pinned by a test) or small style asides, so they are not repeated here.
Automated review - workflow run
8f34a30 to
c431210
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The reader-timezone handling is correct and matches the java.time/Spark reference. Offset-less TIMESTAMP partition values resolve in the connector-supplied reader timezone (UTC by default), embedded offsets and named zones take precedence through Arrow's string_to_datetime, and TIMESTAMP_NTZ and DATE stay timezone-independent. DST overlap resolves to the earlier instant via first.min(second), and gap resolution applies the pre-transition offset to the original local time, which is algebraically the java.time forward-shift rule and holds for the one-hour Los_Angeles gap and the full-day Pacific/Apia skip.
The default path routes through string_to_datetime with the transition-free UTC zone, so the accepted-format set for already-written tables is unchanged. Serde and proto stay backward compatible (default options omitted, existing tags preserved), the proto direction is emission-only, expression transforms preserve options via expr.options.clone(), and the DataFusion and FFI adapters fail closed on any configured option. Every production call site (data skipping, log replay, scan plan, checkpoint transform) still passes MapToStructOptions::default(), so live scan and checkpoint behavior is unchanged.
One candidate blocker was checked and dropped: a claim that visit_unknown_name in ffi/src/expressions/engine_visitor.rs panics via .unwrap() on malformed inbound C string data at an FFI boundary. That callback is defined inside the #[cfg(test)] mod tests block and is only registered into the test visitor, so it is excluded from production builds and unwrap() is idiomatic there; the disprove gate confirmed this is not a present defect.
All six reviewers and the disprove gate completed. Remaining reviewer notes were minor and either already raised in prior review rounds (duplicate is_default() guard wrappers across the datafusion and ffi crates, validation altitude of with_timestamp_timezone, parse_digits fixed-width params, the duplicated expected_timestamp_micros test helper, DATE zone-independence not pinned by a test) or a small cosmetic aside about Expression::Display branching on one options field, so they are not repeated here.
Automated review - workflow run
3 participants
Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
Delta partition values are stored as strings, and
MapToStructis the expression Kernel uses to turn those strings into typed values. An offset-lessTIMESTAMPneeds the reader timezone to produce the same instant as the connector, while the existing behavior assumes UTC.Kernel never reads the machine timezone or infers a writer timezone from table metadata. The connector supplies its reader or session timezone through
MapToStructOptions; default options preserve UTC. The option accepts either an IANA zone, whose offset can change across partition timestamps, or a fixed offset in+HH:MMor-HH:MMform when the reader configuration itself is fixed.Ordinary timestamp strings continue through Arrow's existing
string_to_datetimeparser, preserving the default UTC path and its accepted formats. Kernel only adds handling for an offset-less local time that is repeated or skipped by a clock transition in the configured named zone. An offset or named zone stored in the value still takes precedence, andTIMESTAMP_NTZremains timezone-independent.This PR makes the existing
Expression::map_to_structconstructor requireMapToStructOptions, preserves the options through serde, expression transforms, and the existing declarative-plan message, and evaluates them in Kernel's Arrow engine. DataFusion and expression-visitor FFI remain default-only and reject configured options rather than silently changing their meaning.How to review
The diff has 862 changed lines: roughly 295 lines of core implementation, 110 lines of adapter, wire-format, and call-site scaffolding, and 455 lines of tests. Most of the test code covers timestamp parsing, clock transitions, invalid options, serialization, transforms, and adapter behavior.
kernel/src/expressions/mod.rs.kernel/src/engine/arrow_expression/timestamp_timezone.rs, especially the Arrow-first parsing path and narrow clock-transition fallback.This PR affects the following public APIs
Expression::map_to_structnow requiresMapToStructOptions, andMapToStructExpressionstores the options.How was this change tested?
Unit tests cover timezone parsing, Kernel evaluation, serialization, declarative plans, expression traversal, and adapter behavior.