feat!: honor reader timezone in partition values - #3119
Conversation
DrakeLin
changed the title
github-actions
Bot
added
the
breaking-change
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3119 +/- ##
==========================================
+ Coverage 90.32% 90.38% +0.05%
==========================================
Files 250 251 +1
Lines 89038 89653 +615
Branches 89038 89653 +615
==========================================
+ Hits 80422 81029 +607
+ Misses 5682 5675 -7
- Partials 2934 2949 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 4 · ☑️ 7 · 🚧 4 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
be88139 to
d37783d
Compare
DrakeLin
changed the title
4e85633 to
0994bf4
Compare
DrakeLin
changed the title
1973270 to
650c659
Compare
DrakeLin
changed the title
a84614d to
bcd0d72
Compare
DrakeLin
changed the title
bcd0d72 to
a512f72
Compare
cb70b31 to
e078fed
Compare
DrakeLin
changed the title
d90d2c1 to
5266568
Compare
DrakeLin
changed the title
ef0f76d to
10111fe
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 reader timezone through partition-value parsing. The TimestampTimezone module (offset precedence, embedded-zone handling, DST fold/gap resolution) is careful and well tested, and the proto tag split that forces option-unaware executors to reject configured expressions is a good call. Two issues are worth resolving before merge.
Blocking issues
Blocker1 — kernel/src/checkpoint/checkpoint_shape.rs:42 (with the pruning path in kernel/src/scan/scan_plan.rs)
Checkpoint pruning uses a checkpoint's native add.partitionValues_parsed values, and parsed_partition_values_schema is populated whenever those values are type-compatible (schema_has_compatible_partition_values_parsed checks types only, not the timezone the values were parsed in). The partition predicate is applied against those native values, and pushed into parquet footer/row-group skipping, before partitionValues_parsed is rebuilt from the raw map in the reader timezone. For a zoned TIMESTAMP partition column, native values encode a writer/UTC instant; under a non-UTC reader timezone a file whose reader-timezone instant satisfies the predicate but whose native instant does not is skipped and never re-evaluated, silently dropping matching rows. This is reachable even for kernel-written checkpoints, since the partition schema is passed to CheckpointShape::try_new unconditionally rather than being gated on a non-UTC timezone. The disprove gate confirmed the pre-reparse ordering at scan_plan.rs:176 vs scan_plan.rs:189.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer
Suggested fix: when a non-default timestamp_timezone is configured and the partition schema contains a zoned TIMESTAMP column, treat the native partitionValues_parsed as incompatible for that column so pruning falls back to the raw-map reparse, or restrict native-parsed pruning to non-timestamp columns.
Blocker2 — kernel/src/engine/arrow_expression/evaluate_expression.rs:393 (new parser in kernel/src/timestamp_timezone.rs)
The removed parse_partition_scalar special-cased Date (arrow Date32Type::parse) and TimestampNtz (arrow string_to_datetime), accepting supersets such as 20240115 for dates and T-separated or offset-bearing forms for TIMESTAMP_NTZ. The new parse_partition_scalar special-cases only Timestamp; Date and TimestampNtz now go through PrimitiveType::parse_scalar, which accepts only %Y-%m-%d and %Y-%m-%d %H:%M:%S%.f. Partition values from lenient or non-canonical writers that previously read can now hard-error. Spec-compliant writers only emit canonical forms, so the impact is bounded to non-canonical inputs, but this read-behavior change is not called out.
Raised by: maintainer-claude-reviewer
Suggested fix: retain the lenient arrow parsers for Date and TimestampNtz, or if the narrowing is intentional, document it as a breaking change and note it affects only non-canonical partition values.
Non-blocking notes
Nit1 — docs/user-guide/src/reading/scan_metadata.md:289
The user guide says only "An explicit offset in a partition value takes precedence," but parse_timestamp also honors an embedded IANA zone name in the value, and every other doc for this feature says "a time zone or offset embedded in a value takes precedence."
Raised by: docs-reviewer
Suggested fix: change to "An explicit offset or embedded time zone in a partition value takes precedence."
Nit2 — kernel/src/engine/arrow_expression/evaluate_expression.rs:392
The default MapToStruct path runs TimestampTimezone::parse("UTC") on every evaluation. Branch to TimestampTimezone::default() for the None case to skip the parse.
Raised by: maintainer-claude-reviewer
Nit3 — datafusion-executor/src/expression.rs:523
KernelMapToStructUdf::invoke_with_args clones the full output schema into a KernelDataType on every batch. Precompute the output type (or the kernel expression) once in try_new.
Raised by: maintainer-claude-reviewer
Nit4 — kernel/src/checkpoint/checkpoint_shape.rs:42
Every unit test in this module still passes None for the new partition_schema argument, so the partition-only footer-read trigger and the incompatible-schema to None fallback are only covered indirectly by integration tests.
Raised by: test-coverage-reviewer
Suggested fix: extend the existing rstest matrix with a partition-schema axis asserting the footer read count and the compatible/incompatible resolution.
Summary
The timezone parsing itself is spec-aligned and thoroughly tested. The main risk is Blocker1: checkpoint pruning decides skips on native writer/UTC parsed values while the surviving-row predicate uses reader-timezone semantics, which can drop matching files for zoned TIMESTAMP partitions. Blocker2 is a bounded read regression for non-canonical Date and TIMESTAMP_NTZ partition values that should be fixed or documented. The remaining notes are minor.
Automated review - workflow run
f08a8e3 to
fcebfeb
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads a connector-supplied reader timezone through map-to-struct partition parsing for full snapshot scans. The timezone parser handles IANA zones, fixed offsets, DST overlap/gap/skipped-day cases, embedded-zone precedence, and TIMESTAMP_NTZ invariance, and the coverage for those cases is thorough. The wire-compat split (default semantics stay on proto tag 12, configured semantics move to a new tag 14 so options-unaware executors reject rather than misparse) and the unsafe FFI plumbing are done carefully. There is one blocking correctness gap in early checkpoint partition pruning.
Blocking issues
Blocker1: Early checkpoint/footer partition pruning evaluates the reader-timezone predicate against the checkpoint's native partitionValues_parsed, which was parsed in the writer frame (Spark uses the writer session timezone; kernel's own checkpoint construction uses UTC via build_partition_values_parsed_expr). For an offset-less TIMESTAMP partition column with a non-UTC configured reader timezone, a file whose reader-timezone reparsed value matches the predicate can be pruned before reparsing, so it is never read and matching rows are silently dropped.
- Locations:
kernel/src/scan/scan_plan.rs(checkpoint_arm early filter, ~L176-194,build_actions_partition_predicate),kernel/src/scan/mod.rs(build_actions_meta_predicate, ~L1182-1238, the parquetmeta_predicaterow-group skip overadd.partitionValues_parsed), andkernel/src/checkpoint/checkpoint_shape.rs(try_new_leaf/try_new_manifestaccept a native partition schema viaschema_has_compatible_partition_values_parsed, a type-only check that does not excludeTIMESTAMPcolumns). The final projection reparses surviving rows in the reader timezone, but the early filter has already run against the divergent native frame. The PR's own parity test constructs a timestamp that is rejected in UTC but accepted after reader-timezone parsing and documents that native pruning runs first, and the disprove pass confirmed the discarded rows cannot be recovered downstream. - Raised by: delta-protocol-reviewer, maintainer-claude-reviewer
- Suggested fix: exclude offset-less
TIMESTAMPpartition columns from native early pruning when a non-default reader timezone is configured (both the declarativebuild_actions_partition_predicatepath and the imperativemeta_predicatepath), and rely on the reader-timezoneDataSkippingFilterpass over the reparsed values. Add a test that applies a timestamp partition predicate under a non-UTC reader timezone across thenative_checkpointaxis and asserts the file survives when the reader-timezone value matches.
Non-blocking notes
Nit1: parse_partition_date now accepts full timestamp forms for a DATE target (length > 10 parses as a timestamp and takes the UTC date_naive()), which is broader than the removed Date32Type::parse path. The behavior is intentional and tested, but the doc comment understates it. State that date extraction is deliberately UTC-based and is a superset of the canonical yyyy-MM-dd form.
- Location:
kernel/src/timestamp_timezone.rs(parse_partition_date) - Raised by: maintainer-claude-reviewer
- Suggested fix: add a one-line note to the doc comment describing the accepted superset and the UTC basis.
Nit2: The user guide does not state that the reader timezone affects only zoned TIMESTAMP while TIMESTAMP_NTZ stays wall-clock, and the PartitionValuesOptions::with_timestamp_timezone rustdoc omits the "partition predicate evaluation after log replay" effect that the guide and PR description both call out.
- Location:
docs/user-guide/src/reading/scan_metadata.md(new timezone paragraph),kernel/src/scan/mod.rs(with_timestamp_timezonerustdoc) - Raised by: docs-reviewer
- Suggested fix: add the zoned-vs-NTZ distinction to the guide and the predicate-evaluation effect to the rustdoc so the two stay parallel.
Nit3: CDF and incremental scans parse partition values in UTC regardless of the reader timezone, so a CDF read and a snapshot read of the same offset-less TIMESTAMP partition column under a non-UTC reader timezone return different instants. This is intentional per the PR scope but is not documented.
- Location:
kernel/src/table_changes/physical_to_logical.rs(usesTimestampTimezone::default()) - Raised by: delta-protocol-reviewer
- Suggested fix: note the CDF/incremental UTC-only behavior in the user guide alongside the snapshot behavior.
Nit4: Beyond the timestamp-predicate gap in Blocker1, the parallel_scan_metadata path is exercised only by a processor serde round-trip, not end to end with a non-default reader timezone.
- Location:
kernel/src/scan/mod.rs(parallel_scan_metadata) - Raised by: test-coverage-reviewer
- Suggested fix: add a parallel-path integration test mirroring the existing JSON/checkpoint timezone test and assert the typed timestamp values.
Summary
The timezone parsing core is well designed and consistent with Delta Spark, and edge-case coverage is strong. The one item to resolve before merge is the checkpoint early-pruning soundness gap: for offset-less TIMESTAMP partition columns with a non-UTC reader timezone, early pruning against native partition values can drop files that match under the reader-timezone semantics this PR introduces. The remaining notes are documentation and test-coverage improvements.
Automated review - workflow run
fcebfeb to
412ef8c
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change is well structured and unusually well tested: the timezone engine, the two-tag proto split that makes options-unaware executors reject configured semantics, and the shared parsing leaf are all solid. There is one blocking correctness issue in the checkpoint pruning path.
Blocking issues
Blocker1 - Checkpoint footer pruning can silently drop files that match under the reader timezone.
Location: read path in kernel/src/scan/mod.rs (build_actions_meta_predicate) and kernel/src/scan/data_skipping.rs (as_checkpoint_skipping_predicate); behavior documented at docs/user-guide/src/reading/scan_metadata.md around line 292.
Failure mode: the reader-timezone applies only to the reparse of surviving rows, but the timestamp partition predicate is pushed as an exact min=max footer predicate against the checkpoint's native partitionValues_parsed column, which Delta Spark materialized under the checkpoint writer's session timezone. When the configured timestamp_timezone differs from the writer's (the headline use case for a non-UTC reader), the two interpretations disagree: writer UTC parses 2024-06-15 09:30:00 to 09:30Z, while a reader in America/Los_Angeles reparses the same raw string to 16:30Z. A predicate expressed in reader-timezone instants can prune row groups and Add files at the footer that would match after reparse, and those files are never reconsidered, so rows go missing. Timestamp partition columns are not excluded from the native footer predicate (only floating-point columns get special handling). The test at kernel/src/scan/scan_plan/tests.rs:287 deliberately builds a timestamp that native UTC pruning rejects but reader-timezone parsing accepts and asserts it stays eligible for native pruning, which locks in the false negative rather than guarding against it. Delta Spark avoids this because it uses one session timezone for both pruning and materialization.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer (confirmed by disprove-reviewer).
Suggested fix: when a non-default timestamp_timezone is configured, exclude timestamp partition columns from the native partitionValues_parsed footer predicate (still prune other partition types and data-column stats), or mark timestamp columns incompatible in schema_has_compatible_partition_values_parsed so skipping relies only on the reader-timezone reparse. Add an integration test that reads a checkpoint whose native partitionValues_parsed was written in a timezone different from the configured reader timezone and asserts no matching files are dropped.
Non-blocking notes
Nit1 - Equivalent fixed offsets have distinct expression identities.
MapToStructOptions stores the connector timezone string verbatim (kernel/src/expressions/mod.rs, kernel/src/scan/mod.rs), and KernelMapToStructUdf includes the raw options in its hash and equality (datafusion-executor/src/expression.rs). So +00:00, -00:00, and +00:00:00 parse to the same offset but produce distinct options and distinct UDF identities. Evaluation stays value identical and a scan uses one timezone string, so this cannot cause wrong deduplication, only missed expression or plan reuse.
Raised by: maintainer-codex-reviewer.
Suggested fix: canonicalize valid fixed offsets at the with_timestamp_timezone ingestion boundary while keeping the existing deferred error handling for invalid strings.
Nit2 - The general partition-value parser now lives in the timezone module.
parse_partition_scalar in kernel/src/timestamp_timezone.rs parses every primitive partition type, but only the Timestamp arm consults the timezone, and its sibling wrapper parse_partition_value_raw sits in kernel/src/scan/transform_spec.rs. A maintainer looking for partition parsing will not expect it under a timezone-scoped module.
Raised by: architecture-reviewer.
Suggested fix: keep TimestampTimezone and its resolution helpers in timestamp_timezone.rs, and move the general partition-value parser to a partition-focused home shared by both callers.
Nit3 - Two negative-input parsing edges are untested.
An alphabetic but invalid trailing timezone token (for example 2024-01-15 12:30:45 Foobar) and negative or hour-18 fixed-offset rejections (-19:00, -18:00:01, +18:30) are not exercised.
Raised by: test-coverage-reviewer.
Suggested fix: add rstest cases to the existing rejects_invalid_timestamps and rejects_invalid_timezones tests.
Summary
The parsing engine, proto and FFI plumbing, and test coverage are strong, and TIMESTAMP_NTZ, offset precedence, DST resolution, and fixed-offset validation all match the protocol and Delta Spark. The one issue to resolve before merge is that checkpoint footer pruning still runs against the checkpoint's writer-timezone native parsed values while surviving rows are reparsed in the reader timezone, which can prune and silently drop files that the predicate should match. The remaining items are non-blocking.
Automated review - workflow run
412ef8c to
c78dbca
Compare
DrakeLin
changed the title
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Blocking issues
Blocker1: native checkpoint partition skipping can drop files that match the reader-timezone predicate
Location: kernel/src/scan/scan_plan.rs (checkpoint_arm, the pre-reparse partition filter), gated by the compatibility resolution added in kernel/src/checkpoint/checkpoint_shape.rs (parsed_partition_values_schema) and the UTC checkpoint fallback in kernel/src/checkpoint/checkpoint_transform.rs.
The declarative checkpoint path applies the user partition predicate against the checkpoint's native add.partitionValues_parsed values and drops any Add whose predicate is definitively false (the filter keeps predicate OR predicate IS NULL, so it can only remove rows). Only the survivors are then reparsed from the raw partitionValues map using the configured reader timezone, and the final predicate is re-evaluated on those reparsed values.
For an offset-less zoned TIMESTAMP partition column with a non-default with_timestamp_timezone, the native parsed instant differs from the reader-timezone reparse. Kernel-written checkpoints materialize partitionValues_parsed in UTC (checkpoint_transform.rs uses MapToStructOptions::default()), and CheckpointShape gates only on type compatibility, never timezone provenance. Concretely, with reader timezone America/Los_Angeles, raw value 2024-01-01 00:00:00, and predicate ts = 2024-01-01T08:00:00Z: the native value 2024-01-01T00:00:00Z makes the early predicate false and the file is pruned, but the reader-timezone reparse 2024-01-01T08:00:00Z would have matched. The matching file is silently removed. Range predicates on such columns are affected the same way whenever the UTC-to-reader offset moves the native instant across a bound. This is a false-negative prune, which is data loss, not just extra output.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer (disprove gate: CONFIRMED)
Suggested fix: for zoned TIMESTAMP partition columns, do not apply the user partition predicate against the native partitionValues_parsed when a non-default reader timezone is configured. Keep native skipping for DATE, numeric, string, TIMESTAMP_NTZ, and offset-carrying timestamp values, which are timezone-invariant, and rely on the post-reparse prune for offset-less zoned timestamps. Add a checkpoint-skip-vs-reparse cross-consistency test: a kernel-written (UTC) checkpoint plus with_timestamp_timezone("America/Los_Angeles") plus an equality predicate on an offset-less timestamp partition must still return the matching file.
Non-blocking notes
Nit1: setting a reader timezone silently changes non-timestamp parsing semantics in the DataFusion executor
Location: datafusion-executor/src/expression.rs, map_to_struct_to_df_expr.
The lowering branches on map_to_struct.options.is_default(). When any timestamp_timezone is set it routes the entire struct rebuild through KernelMapToStructUdf, which also switches boolean and decimal handling to strict kernel semantics for every field and replaces the pushdown-friendly named_struct with an opaque UDF. The switch is keyed on whether a timezone is set rather than on whether a field actually needs kernel timestamp parsing. The documented divergences only bite on malformed or non-spec-compliant values, so this is a maintainability and optimizer concern rather than a correctness defect on valid data.
Raised by: architecture-reviewer (disprove gate: NITPICK)
Suggested fix: delegate only zoned-timestamp fields to kernel parsing and keep native per-field lowering for the rest, or route both the default and configured paths through one shared parsing path so the two cannot diverge by field type.
Nit2: reader timezone is re-parsed on every batch in the arrow evaluator
Location: kernel/src/engine/arrow_expression/evaluate_expression.rs, the MapToStruct arm.
TimestampTimezone::try_from_options(&m.options) runs once per evaluate_expression call, so the timezone string is re-parsed for each evaluated batch even though the scan already validated it up front. The cost is per batch rather than per row, so this is minor.
Raised by: maintainer-claude-reviewer (disprove gate: NITPICK)
Suggested fix: parse the timezone once at evaluator construction and reuse the parsed value if it ever shows up in profiles.
Summary
The timezone parsing machinery is well built and thoroughly tested: DST gap and overlap handling, embedded-offset and named-zone precedence, TIMESTAMP_NTZ invariance, fixed-offset validation, the proto tag-12/tag-14 split that makes options-unaware executors reject configured semantics, and the FFI plumbing all check out, and test coverage is good. The one blocking concern, raised independently by the protocol and Claude maintainer reviewers and confirmed by the disprove gate, is that early checkpoint partition skipping still runs against native UTC-parsed values while output and final pruning use the reader timezone, so it can drop matching files for offset-less zoned timestamp partitions. Resolve that before merge. Two non-blocking notes cover the executor semantics switch and a small per-batch reparse.
Automated review - workflow run
3f52d63 to
7fec422
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR threads a reader timezone through partition value parsing. The parsing core in kernel/src/timestamp_timezone.rs (IANA vs fixed-offset split, the +/-18:00 bound on configured offsets, embedded offset/zone precedence, and the DST overlap/gap resolution) is correct and well tested, and the TIMESTAMP_NTZ, FFI, proto, and DataFusion UDF paths hold together. There is one blocking issue on the read path.
Blocking issues
Blocker1 - Native checkpoint partition pruning ignores the reader timezone and can drop matching files
- Location: kernel/src/checkpoint/checkpoint_shape.rs (parsed_partition_values_schema resolution, RIGHT ~line 226) and the native pruning applied in kernel/src/scan/scan_plan.rs before timezone-aware reparsing.
- Failure mode: native checkpoint footer / row-group partition skipping evaluates the predicate against the checkpoint's native
add.partitionValues_parsedvalues. For an offset-less TIMESTAMP partition column those native values encode a UTC (writer) interpretation, and kernel itself writes them as UTC in build_partition_values_parsed_expr. The rest of the scan reparses the raw partition string in the configured non-UTC reader timezone. Availability of native pruning is gated only on type compatibility (schema_has_compatible_partition_values_parsed), with no reader-timezone guard. So for raw2024-01-15 00:00:00underAmerica/Los_Angeles, the native value is00:00:00Zbut the reader-timezone reparse is08:00:00Z; a predicatets = 2024-01-15T08:00:00Zis false against the native value and prunes the file. Native pruning runs before reparse, so the row is never recovered. This is the unsound direction (missing data), not conservative over-retention. - Raised by: maintainer-claude-reviewer, delta-protocol-reviewer, test-coverage-reviewer.
- Suggested fix: exclude TIMESTAMP partition columns from native
partitionValues_parsed-based footer/row-group pruning when a non-default reader timezone is configured (keep native pruning for timezone-independent columns and for the UTC default where native and reparsed values coincide), or treat the native parsed values as incompatible in that case. Add a regression test that drives the over-pruning direction: a predicate positioned so the native UTC partition value is outside the range while the reader-timezone value is inside, asserting against an absolute expected row set that the file survives. The current parity test (declarative vs imperative) only exercises the accept direction and cannot catch a shared over-pruning bug.
Non-blocking notes
Nit1 - Fallback keys on options.is_default(), coupling timezone to parser strictness
- Location: datafusion-executor/src/expression.rs, RIGHT line 424.
- map_to_struct_to_df_expr uses
if map_to_struct.options.is_default()to choose nativenamed_structlowering versus the kernel UDF. Configuring an explicit UTC timezone (documented as non-default and serialized as configured) is semantically identical to the default but routes through the UDF, which silently switches the whole field-parsing contract to kernel-exact for boolean spellings, decimal rescale, and malformed timestamps. Consider keying the fallback on the specific capability gap (a configured timestamp_timezone requiring semantics the cast cannot express) rather than on is_default(). - Raised by: architecture-reviewer.
Nit2 - with_timestamp_timezone doc omits partition-predicate evaluation
- Location: kernel/src/scan/mod.rs (PartitionValuesOptions::with_timestamp_timezone doc comment).
- The API doc says the option applies to typed scan metadata and the row transforms used by Scan::execute, but does not mention partition-predicate evaluation after log replay, which the code and the user guide (docs/user-guide/src/reading/scan_metadata.md) confirm is affected. A connector reading only the API doc could assume its partition predicates are unaffected. Add partition-predicate evaluation to the list.
- Raised by: docs-reviewer.
Summary
The timezone parsing engine and its threading through the materialization, RowTransform, and execution paths are correct and align with Delta Spark's session-zone partition semantics. The one item to resolve before merge is that native checkpoint partition pruning still uses UTC native parsed values and is not gated on the configured reader timezone, so a TIMESTAMP partition predicate can silently drop files a reader-timezone scan should return.
Automated review - workflow run
7fec422 to
851eb59
Compare
DrakeLin
changed the title
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Blocking issues
Blocker1 -- Checkpoint native partition pruning interprets timestamp partitions in UTC while output and the final predicate use the reader timezone.
Location: kernel/src/scan/scan_plan.rs:185-192 (the native_partition_predicate filter) and kernel/src/scan/mod.rs build_actions_partition_predicate (~1210). Design statement: docs/user-guide/src/reading/scan_metadata.md:292.
With a non-UTC with_timestamp_timezone and a predicate on a zoned TIMESTAMP partition column, an offset-less partition string parses to instant U in the checkpoint's native partitionValues_parsed column but to U + offset once the surviving rows are reparsed from the raw map. The early filter evaluates the query predicate against the native UTC value, and Predicate::or(predicate, is_unknown) only readmits rows where the native value is null. A file whose reader-timezone value satisfies the predicate can be definitively rejected by its UTC native value and pruned, so its rows are silently dropped from the result. This is a data-skipping soundness break, not just a missed optimization. The PR's own parity test at kernel/src/scan/scan_plan/tests.rs:288-292 encodes the trigger (native value rejects the predicate, reader-timezone reparse accepts it), and the behavior contradicts the invariant stated in kernel/src/expressions/mod.rs:665 that materialization and pruning must not interpret the same value differently.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer, test-coverage-reviewer (confirmed by disprove-reviewer).
Suggested fix: skip native partition pruning for zoned TIMESTAMP partition references when timestamp_timezone is set (restrict build_actions_partition_predicate to non-timestamp partition columns in that case), or reparse the raw map with the reader timezone before applying the pruning filter. Add a test that asserts the surviving file is actually returned, rather than only asserting declarative and imperative agree.
Non-blocking notes
Nit1 -- Reader timezone is validated late, after it can cross the serialization boundary.
PartitionValuesOptions::with_timestamp_timezone and MapToStructOptions::with_timestamp_timezone (kernel/src/expressions/mod.rs:670) accept any string and store it unvalidated. The only validation happens inside Arrow evaluation via TimestampTimezone::try_from_options, so an invalid zone supplied at scan_builder() time surfaces only when scan_metadata is evaluated, possibly after the value has round-tripped through serialized replay state onto another node.
Raised by: architecture-reviewer.
Suggested fix: validate the timezone at scan construction (a fallible build) while keeping the string on the public API so chrono-tz does not leak into the public surface.
Nit2 -- A second temporal parser now covers cases that do not need timezone handling.
kernel/src/timestamp_timezone.rs hand-rolls the full partition date/timestamp grammar for DATE, TIMESTAMP, and TIMESTAMP_NTZ, replacing arrow's Date32Type::parse / string_to_datetime. Only offset-less zoned TIMESTAMP needs the chrono-tz resolver; DATE and TIMESTAMP_NTZ are timezone-independent and gain nothing but a new accepted-format surface that must stay aligned with arrow's.
Raised by: architecture-reviewer.
Suggested fix: keep the arrow parsers for DATE, TIMESTAMP_NTZ, and offset-carrying/UTC TIMESTAMP, and branch into the new resolver only for an offset-less value with a configured named or fixed reader zone.
Summary
The timezone parser is careful and well tested: IANA and normalized fixed offsets, embedded-offset and embedded-zone precedence, the both-present conflict, DST overlap and gap resolution, whole-day transitions, fixed-offset limits, and TIMESTAMP_NTZ invariance are all covered at unit and integration level, and the FFI, proto, and DataFusion UDF plumbing look internally consistent. The one issue to resolve before merge is Blocker1: checkpoint partition pruning runs against native UTC-parsed values while the rest of the pipeline honors the reader timezone, which can silently drop files that a non-UTC reader's predicate should match. The two notes are optional follow-ups.
Automated review - workflow run
| `+HH:MM:SS`, or `-HH:MM:SS` fixed offset when the reader uses another timezone. An explicit offset | ||
| or embedded time zone in a partition value takes precedence. This setting affects typed | ||
| `scan_metadata` output, partition predicate evaluation after log replay, and the partition-column | ||
| row transforms used by `Scan::execute`. Checkpoint footer pruning continues to use the |
There was a problem hiding this comment.
Blocker1 Checkpoint native partition pruning (kernel/src/scan/scan_plan.rs:185-192 via build_actions_partition_predicate) evaluates the query predicate against native partitionValues_parsed, which is parsed in UTC, while surviving rows are reparsed with the reader timezone. With a non-UTC with_timestamp_timezone, an offset-less TIMESTAMP partition value is instant U natively but U+offset after reparse, so a file whose reader-timezone value matches the predicate can be definitively rejected by its UTC value and pruned. The OR is_unknown guard only readmits null values, so matching rows are silently dropped, which breaks data-skipping soundness. The parity test at scan_plan/tests.rs:288-292 encodes this, and it violates the invariant stated at expressions/mod.rs:665. Raised by: delta-protocol-reviewer, maintainer-claude-reviewer, test-coverage-reviewer. Suggested fix: skip native pruning for zoned TIMESTAMP partition columns when timestamp_timezone is set, or reparse with the reader timezone before the pruning filter; add a test asserting the file is retained.
| #[derive(Clone, Debug, Default, Eq, Hash, PartialEq, Serialize, Deserialize)] | ||
| pub struct MapToStructOptions { | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| timestamp_timezone: Option<String>, |
There was a problem hiding this comment.
Nit1 with_timestamp_timezone stores the timezone string unvalidated; the only validation runs deep in Arrow evaluation via TimestampTimezone::try_from_options, so an invalid zone supplied at scan_builder() time surfaces only during scan_metadata, possibly after serialized replay state has crossed to another node. Raised by: architecture-reviewer. Suggested fix: validate the timezone at scan construction (a fallible build) while keeping the string on the public API.
851eb59 to
8b67130
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Blocking issues
Blocker1 - Checkpoint partition pruning and reader-timezone output disagree, dropping files that match the predicate
File: kernel/src/checkpoint/checkpoint_shape.rs:42 (enabling parsed_partition_values_schema); pruning applied in kernel/src/scan/scan_plan.rs (checkpoint arm) and kernel/src/scan/mod.rs.
The declarative checkpoint arm builds the partition-skipping predicate from the checkpoint's native add.partitionValues_parsed values, which are parsed at write time in UTC (kernel-written checkpoints use MapToStructOptions::default()), and applies it to drop rows before surviving rows are reparsed from the raw map in the reader timezone. For an offset-less TIMESTAMP partition value the UTC-parsed instant and the reader-timezone instant differ, so a predicate carrying reader-timezone timestamp literals can be evaluated against the UTC value during pruning and permanently drop an Add action that actually satisfies the predicate under the reader timezone. The later reader-timezone filter re-filters surviving rows but cannot restore rows already eliminated, so the result is silent data loss, not just a perf regression. This is reachable today: a kernel-written UTC checkpoint read with a non-UTC reader timezone. The parity test declarative_metadata_matches_imperative_with_reader_timezone only checks that the declarative and imperative paths agree, and both share the native-UTC pruning, so it cannot catch the over-pruning.
Raised by: delta-protocol-reviewer, test-coverage-reviewer.
Suggested fix: exclude zoned TIMESTAMP partition columns from native checkpoint footer/row pruning and fall back to reparse-then-prune from the raw map for those columns, or reparse in the reader timezone before evaluating the checkpoint skipping predicate. Add an absolute-assertion integration test that scans a checkpointed timestamp-partitioned table under a non-UTC reader timezone and asserts the exact surviving file set (both a keep case and a prune case), rather than comparing two kernel paths.
Non-blocking notes
Nit1 - is_default() couples edge-case parsing semantics to the timezone toggle
File: datafusion-executor/src/expression.rs:424.
map_to_struct_to_df_expr keys on options.is_default() to choose between the native named_struct(cast(...)) lowering and the kernel UDF. Default options and explicit with_timestamp_timezone("UTC") both mean UTC but take different evaluators, which diverge on malformed or non-spec-compliant values (bool spellings, decimal rescale, trailing named timezone). Spec-compliant values are unaffected and the split is documented, so this is a conscious tradeoff. If you want default UTC and explicit UTC to agree on all inputs, route both through the UDF or document is_default() as a semantic selector rather than a pure fast path.
Raised by: architecture-reviewer, maintainer-claude-reviewer.
Nit2 - reader timezone is validated late
File: kernel/src/scan/mod.rs (PartitionValuesOptions::with_timestamp_timezone).
The public builder stores the timezone string unvalidated; an invalid or misspelled timezone is first rejected during scan metadata evaluation (ScanLogReplayProcessor construction / MapToStruct evaluation) rather than at the call site, and the processor keeps both the wire string and the parsed timezone. Validating and normalizing to a checked type at the builder would surface typos earlier and remove the duplicate representation. This matches the documented contract, so it is optional.
Raised by: architecture-reviewer.
Summary
The timezone parsing itself is careful and well tested: DST gap/overlap resolution, fixed-offset normalization and bounds, embedded-zone precedence, TIMESTAMP_NTZ isolation, empty-string handling, and the FFI/UDF identity concerns all check out against PROTOCOL.md and Spark behavior. The one blocking issue is a data-skipping soundness bug: checkpoint pruning runs on the checkpoint's UTC-parsed partition values while output and the final predicate use the reader-timezone reparse, so a reader-timezone timestamp predicate can silently drop matching files. Resolve that (and add an absolute-assertion pruning test) before merge; the two notes are optional.
Automated review - workflow run
| /// The requested partition schema when the checkpoint has a compatible | ||
| /// `add.partitionValues_parsed` struct; `None` when partitions were not requested or no | ||
| /// compatible parsed values exist. | ||
| pub(crate) parsed_partition_values_schema: Option<SchemaRef>, |
There was a problem hiding this comment.
Blocker1 Checkpoint partition pruning uses the checkpoint's native (UTC-parsed) add.partitionValues_parsed while surviving rows are reparsed in the reader timezone (scan_plan.rs checkpoint arm). For offset-less TIMESTAMP partitions the two instants differ, so a reader-timezone predicate can permanently drop Add actions that actually match, which is silent data loss; the later reader-tz filter cannot restore pruned rows. The declarative/imperative parity test shares the native pruning and cannot catch this. Raised by: delta-protocol-reviewer, test-coverage-reviewer. Suggested fix: exclude zoned TIMESTAMP partition columns from native checkpoint pruning (reparse-then-prune from the raw map), or reparse in the reader timezone before the skipping predicate; add an absolute-assertion pruning test under a non-UTC reader timezone.
| let target = require_struct_output(output_type, "MapToStruct")?; | ||
| let map = to_df_expr(&map_to_struct.map_expr, input_schema, None)?; | ||
|
|
||
| if map_to_struct.options.is_default() { |
There was a problem hiding this comment.
Nit1 map_to_struct_to_df_expr selects the parser by options.is_default(): default options use the native named_struct(cast(...)) lowering while any configured option (including explicit with_timestamp_timezone("UTC")) uses the kernel UDF. The two diverge on malformed/non-spec-compliant values (bool spellings, decimal rescale, trailing named timezone), so default UTC and explicit UTC can disagree. Spec-compliant values are unaffected and this is documented. Raised by: architecture-reviewer, maintainer-claude-reviewer. Suggested fix: route both paths through the UDF for exact parity, or document is_default() as a semantic selector.
8b67130 to
30daea1
Compare
30daea1 to
e241228
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Summary
The timezone parsing core is careful and well tested: named-zone vs normalized fixed-offset handling, DST gap and overlap resolution, embedded-offset and named-zone precedence, TIMESTAMP_NTZ isolation, fixed-offset bounds, empty-string handling, proto and UDF identity, and the FFI plumbing all check out against PROTOCOL.md and Spark. No new blocking issues in this diff. The checkpoint native-UTC pruning versus reader-timezone reparse soundness concern from the prior review is still present and unchanged at this head SHA; it remains open and is not re-filed here to avoid a duplicate inline comment, but it should be resolved before merge along with an absolute-assertion keep/prune test under a non-UTC reader timezone. All six reviewers and full coverage were obtained.
Automated review - workflow run
1 participant
🥞 Stacked PR
Use this link to review incremental changes.
Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
Full snapshot scans need the reader timezone when raw partition strings are parsed into typed metadata, final predicate inputs, and partition columns materialized into data rows. Checkpoint footer pruning remains on the checkpoint's native parsed values for compatibility; surviving rows are then reparsed consistently with the connector session.
This PR adds
PartitionValuesOptions::with_timestamp_timezonefor full snapshot scans and threads it through imperative, parallel, and declarative metadata processing, serialized replay processors, per-file RowTransforms, andScan::execute. Connectors that parse raw partition values themselves can use the option withstring_map_only; connectors that consume typed values can use it withwith_struct.Checkpoint footer partition skipping continues to use compatible native
partitionValues_parsedvalues and can remove row groups before raw values are reparsed. Surviving JSON and checkpoint rows rebuild typed partition values from rawpartitionValues, keyed by physical column name, and use the reader-timezone result for final predicate evaluation, typed scan metadata, RowTransforms, and execution.The timezone affects only zoned
TIMESTAMP;TIMESTAMP_NTZremains a wall-clock value, and an explicit offset in a partition value takes precedence. CDF and incremental scans keep their existing raw-map behavior.This PR affects the following public APIs
Adds
PartitionValuesOptions::with_timestamp_timezone.How was this change tested?
Unit and integration tests cover timezone parsing, JSON and checkpoint scans, partition skipping, column mapping, RowTransforms, execution, and error handling.