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.50% 90.51%
========================================
Files 255 255
Lines 91237 91397 +160
Branches 91237 91397 +160
========================================
+ Hits 82574 82727 +153
- Misses 5650 5656 +6
- Partials 3013 3014 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 13 · ☑️ 1 · 🚧 1 · ❌ 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
3f52d63 to
7fec422
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 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.
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
e241228 to
7375350
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues in this diff. The reader-timezone parsing core checks out against PROTOCOL.md and Spark: named-zone vs fixed-offset handling, DST gap and overlap resolution, embedded-offset and named-zone precedence, TIMESTAMP_NTZ isolation, fixed-offset bounds, empty-string handling, physical-name keying, and the FFI and UDF plumbing.
Summary
The timezone parsing itself is careful and well covered, and the FFI, UDF identity, and scan-threading changes are consistent. No new blocking defect appears in this diff, and the one new candidate (the hand-rolled parser requiring seconds in every time-bearing form) is confined to non-spec-compliant partition values, so it is not a correctness problem. The checkpoint footer pruning vs reader-timezone reparse soundness issue that both prior reviews filed is still present and unresolved at this head SHA (native UTC-parsed partitionValues_parsed can drop an Add that would match under the reader timezone); it is not re-filed here to avoid a duplicate inline comment, but it remains open and should be resolved before merge, along with an absolute-assertion keep and prune test under a non-UTC reader timezone. All six reviewers and the disprove gate completed, so coverage is full.
Automated review - workflow run
7375350 to
0b7433a
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues in this diff.
The reader-timezone parsing core in kernel/src/timestamp_timezone.rs checks out against PROTOCOL.md and Spark: named-zone versus fixed-offset handling, DST fold (earlier instant) and spring-forward gap (pre-transition offset, with the 48-hour walk-back), fixed-offset normalization and bounds, embedded offset and named-zone precedence over the reader zone, TIMESTAMP_NTZ isolation, empty-string handling, and subsecond truncation. The single kernel parser is now shared by the arrow evaluator and the per-file row transform, and the scan-metadata, final-predicate, and Scan::execute paths all derive the timezone from one source string, so they do not drift. The FFI plumbing (OptionalValue<KernelStringSlice>, the unsafe marking, invalid-UTF-8 rejection, and the free(NULL)-safe C side) and the DataFusion is_default() split with the KernelMapToStructUdf identity are internally consistent.
Summary
The timezone parsing itself is careful and well covered, and the FFI, UDF identity, and scan-threading changes are consistent. No new blocking defect appears in this diff. The checkpoint footer pruning versus reader-timezone reparse soundness issue that prior reviews filed is still present and unresolved at this head SHA: the checkpoint arm prunes against the checkpoint's native UTC-parsed partitionValues_parsed and only then reparses surviving rows in the reader timezone, so for an offset-less TIMESTAMP partition read under a non-UTC reader zone an Add that matches under the reader timezone can be permanently dropped, and the later filter cannot restore it. It is not re-filed here to avoid a duplicate inline comment, but it remains open and should be resolved before merge, along with an absolute-assertion keep-and-prune integration test under a non-UTC reader timezone rather than a declarative-versus-imperative parity test. Coverage note: the second maintainer pass (maintainer-claude-reviewer) returned only after an extended delay; its result is incorporated and agrees with the other five reviewers, so coverage is full.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues in this diff.
The reader-timezone parser in kernel/src/timestamp_timezone.rs checks out against PROTOCOL.md and Spark: named-zone versus fixed-offset handling, DST fold to the earlier instant, spring-forward gap resolution via the 48-hour walk-back, fixed-offset normalization and bounds, embedded offset and named-zone precedence over the reader zone, TIMESTAMP_NTZ isolation, empty-string handling, and subsecond truncation. Moving the parser to the crate root gives the Arrow evaluator and the per-file row transform one shared implementation, and the scan-metadata, final-predicate, and Scan::execute paths all derive the timezone from one source string. The FFI plumbing (OptionalValue, the unsafe marking with a Safety note, invalid-UTF-8 rejection with no partial allocation, and the free-safe C side), the KernelMapToStructUdf identity (derived Eq over all fields, manual Hash over a valid subset), and the checkpoint_shape parsed_partition_values_schema wiring are internally consistent.
Summary
No new blocking defect appears in this diff, and all six reviewers plus the docs and test passes agree. The one substantive protocol-soundness problem is not new: checkpoint footer pruning still uses the checkpoint's native UTC-parsed partitionValues_parsed and only then reparses surviving rows in the reader timezone, so for an offset-less TIMESTAMP partition read under a non-UTC reader zone an Add that matches under the reader timezone can be permanently dropped and the later filter cannot restore it. Multiple reviewers independently confirmed it is still live at this head SHA (kernel-written UTC checkpoint via checkpoint_transform.rs:216, pruned in scan_plan.rs:186-206 before the reader-timezone reparse). It was already filed as a blocker at this same head SHA, so it is not re-filed here to avoid a duplicate inline comment, but it remains open and should be resolved before merge, along with an absolute-assertion keep-and-prune integration test under a non-UTC reader timezone rather than the declarative-versus-imperative parity test, which shares the native-UTC pruning and cannot catch over-pruning. Coverage note: the maintainer-claude-reviewer pass returned only after an extended delay; its result is incorporated and agrees with the other five reviewers, so coverage is full.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues in this diff.
The reader-timezone parser now living at the crate root (kernel/src/timestamp_timezone.rs) checks out against PROTOCOL.md and Delta Spark: named-zone versus fixed-offset handling, DST fold to the earlier instant, spring-forward gap resolution via the 48-hour walk-back, fixed-offset normalization and bounds, embedded offset and named-zone precedence over the reader zone, TIMESTAMP_NTZ isolation, empty-string handling, and subsecond truncation. parse_partition_scalar is now a single kernel-side primitive shared by the Arrow evaluator and the per-file row transform, so materialization and the typed struct cannot drift. The FFI plumbing (OptionalValue<KernelStringSlice>, the unsafe marking with a Safety note, invalid-UTF-8 rejection before any allocation, and the free(NULL)-safe C side), the KernelMapToStructUdf identity (derived Eq over all fields with a manual Hash over a functionally-determining subset), and the checkpoint_shape parsed_partition_values_schema wiring are internally consistent.
Summary
No new blocking defect appears in this diff, and all six reviewers agree the timezone parser, FFI, UDF identity, checkpoint-shape wiring, and scan threading are sound. The one substantive protocol-soundness concern is not new: checkpoint footer pruning still uses the checkpoint's native UTC-parsed partitionValues_parsed (scan_plan.rs:186-206, fed by checkpoint_transform.rs:216) before surviving rows are reparsed in the reader timezone, so an offset-less TIMESTAMP Add that would match under a non-UTC reader zone can be permanently pruned. Multiple reviewers reconfirmed it is unchanged at this head SHA, where it was already filed as a blocker, so it is not re-filed here to avoid a duplicate, but it remains open and should be resolved before merge, along with an absolute-assertion keep-and-prune integration test under a non-UTC reader timezone (the current declarative-versus-imperative parity test shares the native-UTC pruning and cannot catch over-pruning). Related previously-filed notes (the MapToStructOptions doc overstatement at kernel/src/expressions/mod.rs:665, the now-unconditional chrono-tz dependency, and the two grammars kept in sync) are likewise unchanged and not re-filed. Coverage is full.
Automated review - workflow run
|
|
||
| /// A DataFusion scalar UDF that delegates map-to-struct parsing to kernel's Arrow evaluator. | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| struct KernelMapToStructUdf { |
There was a problem hiding this comment.
Nit1 KernelMapToStructUdf stores output_schema, output_type, and return_type, but the latter two are both derived from output_schema in try_new. Keeping three representations of one schema coherent is future maintenance cost, and the manual Hash plus derived Eq must be reasoned about across all three. Raised by: architecture-reviewer. Suggested fix: store only output_schema and options, and compute output_type and return_type on demand (or memoize a single derived form) so identity and construction have one source of truth.
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues.
Non-blocking notes
Nit1
kernel/src/expressions/mod.rs (around line 731). The MapToStructExpression value-parsing doc still says kernel's reference evaluator implements these rules "with PrimitiveType::parse_scalar and equivalent Arrow parsers for dates and timestamps." This PR removes the Arrow-based date and timestamp parsing from the evaluator: string_to_datetime and Date32Type::parse no longer appear in kernel, and date and timestamp partition values are now parsed by the hand-rolled routines in kernel/src/partition_values.rs (parse_partition_date, parse_timestamp). The PR edited this same doc block (the TIMESTAMP bullet just below), so the stale sub-clause about Arrow parsers is now inaccurate.
Raised by: docs-reviewer.
Suggested fix: reword to reflect kernel's own parsing, for example "with PrimitiveType::parse_scalar and kernel's own date and timestamp parsers, which preserve the forms Arrow accepts."
Summary
The reader-timezone parser in kernel/src/partition_values.rs is careful and well covered: named-zone versus fixed-offset handling, DST fold to the earlier instant, spring-forward gap resolution via the 48-hour walk-back, fixed-offset bounds, embedded offset and named-zone precedence, TIMESTAMP_NTZ isolation, subsecond truncation, and the extended-year date arithmetic all check out against PROTOCOL.md and Delta Spark. The shared parse_partition_scalar primitive backing both the Arrow evaluator and the per-file row transform, the FFI plumbing, the KernelMapToStructUdf identity, and the checkpoint-shape parsed_partition_values_schema wiring are internally consistent. No new blocking defect appears in this diff. One candidate blocker, that the hand-rolled parser rejects fully compact offset-bearing forms the old Arrow path accepted, was disproved: the new tests preserve the dashed-date compact-offset and spaced-offset forms the old tests asserted, and the fully compact form is not a spec-compliant Delta partition serialization. The one substantive protocol-soundness item, checkpoint footer pruning against the checkpoint's native UTC-parsed partitionValues_parsed before the reader-timezone reparse, so an offset-less TIMESTAMP Add that would match under a non-UTC reader zone can be permanently pruned, is real and still open, but it was already filed as a blocker at this same head SHA and the diff does not materially change that behavior, so it is not re-filed here. It should be resolved before merge, along with an absolute-assertion keep-and-prune integration test under a non-UTC reader timezone; the current declarative-versus-imperative parity test shares the native-UTC pruning and cannot catch over-pruning. All six reviewers and the disprove gate completed, so coverage is full.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The reader-timezone parser in kernel/src/partition_values.rs holds up against PROTOCOL.md and Spark: embedded offset and named-zone precedence, DST fold to the earlier instant, spring-forward gap resolution via the walk-back, fixed-offset bounds, subsecond truncation, and TIMESTAMP_NTZ isolation all check out, and the full-day gap path (Pacific/Apia) is already exercised through the shared resolver. Moving the parser to a crate-root module so the Arrow evaluator and the per-file row transform share one implementation is the right cut. The FFI plumbing (OptionalValue, the unsafe marking with a Safety note, invalid-UTF-8 rejection before allocation), the KernelMapToStructUdf Hash/Eq pair, and the checkpoint_shape parsed_partition_values_schema wiring are internally consistent. One candidate blocker, that compact-clock plus named-timezone strings are mis-split by the rfind heuristic, was disproved: rfind splits at the final whitespace, so the compact prefix still parses.
The checkpoint footer pruning versus reader-timezone reparse soundness concern (an offset-less TIMESTAMP Add that would match under a non-UTC reader zone can be pruned against the checkpoint's native UTC-parsed partitionValues_parsed before the reparse) remains real and open, but it was already filed at this same head SHA and this diff does not change that behavior, so it is not re-filed here. It should still be resolved before merge, along with an absolute-assertion keep-and-prune integration test under a non-UTC reader timezone; the current declarative-versus-imperative parity test shares the native-UTC pruning and cannot observe over-pruning.
Non-blocking notes
Nit2
kernel/src/scan/log_replay.rs:259. The reader timezone is validated twice on two currencies: ScanLogReplayProcessor::new derives a validated TimestampTimezone for the imperative path, while the raw Option<&str> is threaded into the transform expression and re-validated at eval time via TimestampTimezone::try_from_options. Both sites call the same parser today, but the two validation points could drift, which is the exact divergence the shared-parser design aims to prevent. This is a maintenance note, not a present defect.
Raised by: architecture-reviewer.
Suggested fix: route the imperative path through the same options currency (or share a single parsed result) so there is one validation point of record.
Automated review - workflow run
| skip_stats, | ||
| synthesize_json, | ||
| } = stats_options; | ||
| let timestamp_timezone_name = partition_values_options.timestamp_timezone.as_deref(); |
There was a problem hiding this comment.
Nit2 The reader timezone is validated twice on two currencies: this constructor derives a validated TimestampTimezone for the imperative path, while the raw Option<&str> is threaded into the transform expression and re-validated at eval time via TimestampTimezone::try_from_options. Both sites call the same parser today, but the two validation points could drift, the exact divergence the shared-parser design aims to prevent. Maintenance note, not a present defect. Raised by: architecture-reviewer. Suggested fix: route the imperative path through the same options currency (or share a single parsed result) so there is one validation point of record.
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
The reader-timezone parser and its wiring hold up well. The protocol reviewer confirmed embedded-offset and named-zone precedence, TIMESTAMP_NTZ isolation, DST ambiguous-to-earlier-instant and gap-to-prior-offset resolution, subsecond truncation, and fixed-offset bounds all match PROTOCOL.md and Delta Spark. The FFI options plumbing, the KernelMapToStructUdf identity, and the checkpoint-shape partition schema wiring are internally consistent, and test coverage of the materialized and typed surface is good. One blocking issue remains: the PR leaves a test call at the old two-argument arity, which will not compile.
Blocking issues
Blocker1
kernel/src/scan/transform_spec.rs:470-471. This PR widens parse_partition_value_raw to a three-argument signature (raw, data_type, timestamp_timezone) (definition at line 255), and every other call site in the file and in scan/tests.rs was updated to pass the new TimestampTimezone argument. This one call site was missed:
let timestamp_value =
parse_partition_value_raw(Some(&empty), &DataType::TIMESTAMP).unwrap();It still passes only two arguments, so the #[cfg(test)] mod tests block does not compile. That breaks cargo test and the cargo clippy --tests pre-push gate.
Raised by: maintainer-claude-reviewer (confirmed by disprove gate).
Suggested fix: pass the timezone argument like the sibling call sites:
let timestamp_value =
parse_partition_value_raw(Some(&empty), &DataType::TIMESTAMP, TimestampTimezone::default())
.unwrap();Summary
The change is sound in shape and correctness: the shared TimestampTimezone primitive at the crate root, the FFI options handle, the DataFusion UDF, and the checkpoint-shape wiring all check out, and reviewers found no new protocol or safety defect in this diff. There is one blocking compile error in a test call at transform_spec.rs:471 that must be fixed. The previously filed items (checkpoint footer pruning against native UTC partition values before the reader-timezone reparse, the two-currency reader-timezone validation, the parsed_partition_values_expr home, the redundant KernelMapToStructUdf fields, and the stale Arrow-parser doc sub-clause) are unchanged at this head SHA and are not re-filed. A candidate panic in truncate_subseconds was disproved: the digits <= 9 guard returns before any slicing when a non-ASCII byte follows the decimal point. All six reviewers and the disprove gate completed, so coverage is full.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues.
The reader-timezone parser moved to the crate-root kernel/src/partition_values.rs holds up. The protocol reviewer confirmed embedded-offset and named-zone precedence, DST ambiguous-to-earlier-instant and gap-to-prior-offset resolution, fixed-offset bounds, subsecond truncation, date-only handling, and TIMESTAMP_NTZ isolation all match PROTOCOL.md and Delta Spark. The FFI FfiMapToStructOptions plumbing (borrowed slice form, invalid-UTF-8 rejection before allocation, matched unsafe signature at both call sites), the KernelMapToStructUdf Hash/Eq pair and invoke path, the CheckpointShape parsed_partition_values_schema wiring, and the ScanPartitionValuesOptions loss of Copy are all internally consistent and complete. Test coverage of the parser edge cases, FFI round-trip, the UDF, and checkpoint-shape resolution is good.
Two candidates raised this run were both disproved. The truncate_subseconds byte-slicing panic candidate is disproved: find('.') lands on an ASCII boundary and slicing only occurs after a run of ASCII digits, so decimal + 10 and decimal + 1 + digits are always char boundaries. The FFI null-pointer dereference candidate is disproved: visit_expression_map_to_struct is unsafe with a documented pointer-validity contract, matching sibling FFI functions that rely on the same precondition rather than null-checking required inputs.
The previously filed items are unchanged at this head SHA and are not re-filed: the two-argument parse_partition_value_raw test call at kernel/src/scan/transform_spec.rs:471 that will not compile, the checkpoint footer pruning that prunes offset-less TIMESTAMP Adds against native UTC partitionValues_parsed before the reader-timezone reparse (with its accompanying absolute keep-and-prune test gap), the home of parsed_partition_values_expr and the two-currency reader-timezone validation, and the stale Arrow-parser sub-clause in the MapToStructExpression doc. These remain worth resolving before merge. All six reviewers and the disprove gate completed, so coverage is full.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
Blocking issues
Blocker1
kernel/src/scan/transform_spec.rs:409 and :474. This PR widens parse_partition_value_raw to a three-argument signature (raw, data_type, timestamp_timezone) (definition at line 255). Two calls inside the #[cfg(test)] mod tests block still pass only two arguments, so the test module does not compile and both cargo test and the cargo clippy --tests gate fail:
// line 409
assert!(parse_partition_value_raw(Some(&invalid_timestamp), &DataType::TIMESTAMP).is_err());
// line 474
let timestamp_value =
parse_partition_value_raw(Some(&empty), &DataType::TIMESTAMP).unwrap();The :474 site was reported in a prior review. The :409 site (in test_parse_partition_value_raw_protocol_timestamp) is a separate, previously-unreported call site, so fixing only the earlier-flagged line still leaves the module uncompilable. Every production call site and scan/tests.rs were updated to the new arity; only these two test calls were missed.
Raised by: maintainer-claude-reviewer, delta-protocol-reviewer.
Suggested fix: pass the timezone argument at both sites, matching the sibling calls:
assert!(parse_partition_value_raw(
Some(&invalid_timestamp),
&DataType::TIMESTAMP,
TimestampTimezone::default(),
)
.is_err());
let timestamp_value =
parse_partition_value_raw(Some(&empty), &DataType::TIMESTAMP, TimestampTimezone::default())
.unwrap();Non-blocking notes
Nit2
kernel/src/checkpoint/checkpoint_shape.rs. try_new, from_v2_checkpoint_hint, try_new_manifest, and try_new_leaf now each take a second positional partition_schema: Option<&SchemaRef> beside stats_schema, and the partition-compatibility filter blocks in try_new_leaf and try_new_manifest are line-for-line twins of the stats blocks. A third typed-metadata kind would touch all four signatures and add a third duplicated block. The Option-per-kind positional arguments are also easy to transpose at call sites. This is a maintenance note, not a defect; two kinds is where the pattern is visible but a fold is not yet required.
Raised by: architecture-reviewer.
Suggested fix: pass a single borrowed request struct (for example TypedMetadataRequest { stats, partitions }) or iterate a small [(requested_schema, compatibility_fn)] slice so the footer-compatibility check exists once.
Nit3
kernel/src/checkpoint/checkpoint_shape.rs. The new FileType::Parquet if stats_schema.is_some() || partition_schema.is_some() condition in the empty-sidecars leaf branch of from_v2_checkpoint_hint has a partition-only half that is not directly tested. partition_only_request_triggers_manifest_footer_read covers the manifest branch, but no leaf-path test asserts that a partition-only request (stats_schema = None, partition_schema = Some(..)) reads the leaf footer exactly once.
Raised by: test-coverage-reviewer.
Suggested fix: add a leaf/empty-sidecars-hint analog of partition_only_request_triggers_manifest_footer_read that asserts footer_reads == 1 for a partition-only request.
Summary
The reader-timezone work is sound in shape and protocol behavior. The protocol reviewer confirmed offset-less values interpreted in the configured zone, explicit-offset precedence, TIMESTAMP_NTZ isolation, DST ambiguous-to-earlier and gap-to-prior-offset resolution, the 18 hour offset bound, and micros handling all match PROTOCOL.md and Spark, and that the forms the new hand-rolled parser drops relative to the old Arrow path are outside the spec grammar. The FFI options plumbing, the KernelMapToStructUdf identity and execution, and the CheckpointShape partition-schema wiring are internally consistent and well tested. One blocking compile error remains: two test calls to parse_partition_value_raw still use the old two-argument arity, and the newly-found :409 site means fixing only the previously-reported line leaves the test module uncompilable. The checkpoint footer pruning soundness concern (offset-less TIMESTAMP Adds pruned against native UTC partitionValues_parsed before the reader-timezone reparse) and its absolute keep-and-prune test gap were filed as a blocker in prior reviews at this same head SHA and are unchanged by this diff, so they are not re-filed here but should still be resolved before merge. All six reviewers and the disprove gate completed, so coverage is full.
Automated review - workflow run
|
|
||
| By default, offset-less `TIMESTAMP` partition strings are interpreted in UTC. Use | ||
| `with_timestamp_timezone` with a recognized IANA timezone or a normalized `+HH:MM`, `-HH:MM`, | ||
| fixed offset when the reader uses another timezone. An explicit offset or embedded time zone in a |
There was a problem hiding this comment.
Nit1 The added text says "An explicit offset or embedded time zone in a partition value takes precedence." As changed, the parser only honors an explicit numeric offset in the ISO 8601 (T-separated) form. A trailing named zone such as 2024-01-15 12:30:45 America/New_York is now rejected, not honored: the new tests move that case into rejects_unsupported_partition_timestamps, and parse_explicit_offset_timestamp requires byte 10 to be T. This also disagrees with the sibling docs in kernel/src/partition_values.rs and kernel/src/expressions/mod.rs, which say only "explicit offset". Raised by: docs-reviewer. Suggested fix: drop "or embedded time zone", e.g. "An explicit offset in a partition value takes precedence."
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
The reader-timezone rework holds up. The protocol reviewer confirmed offset-less space-separated TIMESTAMP values are interpreted in the configured zone (UTC default), explicit-offset ISO 8601 values are read as absolute instants with precedence, TIMESTAMP_NTZ stays timezone-independent, and the hand-rolled parser implements DST ambiguous-to-earlier and gap-to-prior-offset, the 18 hour bound, and out-of-grammar rejection without panics. The maintainer passes verified the parser byte-slicing is panic-safe, the ScanPartitionValuesOptions serde default preserves backward compatibility, and the checkpoint_shape and scan_plan wiring is internally consistent. One new non-blocking doc inaccuracy was found.
Summary
No new blocking issues. The change is protocol-faithful and the wiring checks out across the maintainer, protocol, architecture, and test-coverage passes. Two blocker-class items raised this run (the two-argument parse_partition_value_raw test calls at transform_spec.rs:409 and :474 that will not compile, and the checkpoint native-UTC partition pruning that runs before the reader-timezone reparse in scan_plan.rs checkpoint_arm, with its keep-and-prune test gap) were already filed in prior AI reviews at this same head SHA and are unchanged by this diff, so they are not re-filed; both should still be resolved before merge. All six reviewers completed; no disprove gate was required because there are no new blocking findings, so coverage is full.
Automated review - workflow run
| /// Interpret offset-less zoned `TIMESTAMP` partition strings in `timestamp_timezone`. | ||
| /// | ||
| /// `timestamp_timezone` must be an IANA time zone identifier recognized by Kernel or a | ||
| /// normalized fixed offset in `+HH:MM` or `-HH:MM` form. A time zone or offset embedded in a |
There was a problem hiding this comment.
Nit1 The doc says "A time zone or offset embedded in a partition value takes precedence," but the new parser only honors an explicit numeric offset in the ISO 8601 T-separated form; a trailing named zone like 2024-01-15 12:30:45 America/New_York is now rejected (see parse_explicit_offset_timestamp requiring byte 10 == T, and the case moved into rejects_unsupported_partition_timestamps). This also disagrees with the sibling docs in partition_values.rs and expressions/mod.rs, which say only "explicit offset."
Raised by: docs-reviewer.
Suggested fix: drop "A time zone or", e.g. "An explicit offset in a partition value takes precedence."
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No new blocking issues.
The reader-timezone rework holds up across all six passes. The protocol reviewer confirmed offset-less TIMESTAMP partition values are interpreted in the configured zone (UTC default), explicit-offset ISO 8601 values are read as absolute instants with precedence, TIMESTAMP_NTZ stays timezone-independent, and the hand-rolled parser in kernel/src/partition_values.rs implements DST ambiguous-to-earlier and gap-to-prior-offset, the 18 hour bound, <=6-digit micros, and panic-free out-of-grammar rejection, all matching PROTOCOL.md. The two maintainer passes verified the parser byte access is bounds-checked (.get(10), split_once, slice-pattern), the ScanPartitionValuesOptions serde default and loss of Copy are backward-compatible and compile, the KernelMapToStructUdf output-type hoist preserves the Hash/Eq contract, and the CheckpointShape partition-schema widening and scan_plan.rs native-prune-then-reparse wiring are internally consistent with all production call sites updated. The architecture pass found the dominant shape move (unifying typed partition values on the raw map and confining native parsed values to pruning) a net reduction in complexity.
Three items confirmed real and present in this diff were already filed in prior AI reviews at this same head SHA and are unchanged, so they are not re-filed here, but they remain open and should be resolved before merge: (1) the two-argument parse_partition_value_raw test calls at kernel/src/scan/transform_spec.rs:409 and :474 that do not match the widened three-argument signature and fail test compilation; (2) the checkpoint native-UTC partitionValues_parsed pruning that runs before the reader-timezone reparse in scan_plan.rs checkpoint_arm, whose keep-and-prune retention path is never exercised because no test checkpoint carries a compatible native partitionValues_parsed; and (3) the doc wording in kernel/src/scan/mod.rs and docs/user-guide/src/reading/scan_metadata.md claiming an embedded named time zone takes precedence, which the parser now rejects.
All six reviewers completed, so coverage is full. No disprove gate was required because there are no new blocking findings this run.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
The reader-timezone rework holds up across all six reviewer passes. Offset-less TIMESTAMP partition values are interpreted in the configured timezone (UTC by default), explicit-offset ISO 8601 values are read as absolute instants with precedence, and TIMESTAMP_NTZ stays timezone-independent. The protocol reviewer traced the hand-rolled parser in kernel/src/partition_values.rs and confirmed DST ambiguous-to-earlier and gap-to-prior-offset resolution, the 18 hour offset bound, the <=6-digit micros pre-check, and panic-free out-of-grammar rejection, all matching PROTOCOL.md. The two maintainer passes verified the parser byte access is bounds-checked, the loss of Copy on ScanPartitionValuesOptions and its serde default are backward-compatible, the KernelMapToStructUdf output-type hoist preserves the Hash/Eq contract, and the CheckpointShape partition-schema widening plus the scan_plan.rs native-prune-then-reparse wiring are internally consistent with all production and test call sites updated. The checkpoint deferral (excluding zoned TIMESTAMP fields from native pre-reparse pruning and applying the native predicate conservatively via OR with is_null before reparsing from the raw map) is sound, and the new over-prune integration test exercises the UTC-checkpoint hazard. The architecture pass judged confining native partitionValues_parsed to pruning and materializing typed values from the single raw map a net reduction in complexity.
Two items carried by prior AI reviews at an earlier head SHA are already corrected in the current checkout and are not re-filed: the two-argument parse_partition_value_raw test calls in kernel/src/scan/transform_spec.rs now pass the three-argument form, and the doc wording in kernel/src/scan/mod.rs and docs/user-guide/src/reading/scan_metadata.md now reads "An explicit offset in a partition value takes precedence." The two test-coverage gaps the test reviewer noted (the checkpoint native-predicate keep-and-prune fold and the partition-only leaf footer-read clause) remain accurate but are duplicates of findings already filed on these same unchanged code paths, so they are suppressed here; both are still worth closing before merge.
All six reviewers completed, so coverage is full. No disprove gate was required because no new blocking or should-fix findings were raised.
Automated review - workflow run
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change is protocol-faithful in the primary snapshot-scan path: the hand-rolled parser is panic-free and bounds-checked, offset-less TIMESTAMP strings are interpreted in the configured timezone (UTC default), explicit-offset ISO 8601 values are read as absolute instants with precedence, and TIMESTAMP_NTZ stays timezone-independent. Two correctness gaps came out of the reader-timezone rework, both confirmed by the disprove gate.
Summary
The parser rewrite and the configured-timezone deferral are sound, but decoupling typed output from checkpoint footer pruning introduced two confirmed correctness gaps: default-mode TIMESTAMP pruning against a foreign non-UTC checkpoint can drop matching files, and the declarative-plans path silently ignores the configured timezone. Both should be resolved before merge. All six reviewers plus the disprove gate completed, so coverage is full.
Automated review - workflow run
| for field in schema.fields() { | ||
| // Native checkpoint values may encode a different instant than reader-timezone | ||
| // parsing, so they cannot safely prune zoned timestamp partitions. | ||
| if self.partition_values.timestamp_timezone.is_some() |
There was a problem hiding this comment.
Blocker1 Native checkpoint TIMESTAMP footer pruning is deferred only when a timezone is configured. In default (UTC) mode the guard does not fire, but typed output/selection now always reparse the raw partitionValues map as UTC while footer pruning still evaluates the predicate against the checkpoint's native partitionValues_parsed column (schema_has_compatible_partition_values_parsed accepts TIMESTAMP). A foreign checkpoint written in a non-UTC session stores writer-zone instants, so a predicate matching the UTC-reparsed value can prune the row group and silently drop a matching file (data-skipping false negative). Pre-PR output and pruning shared the native column, so they were consistent.
Raised by: delta-protocol-reviewer (untested branch also noted by test-coverage-reviewer).
Suggested fix: make the TIMESTAMP deferral unconditional (drop the timestamp_timezone.is_some() conjunct), since output/selection now always derive from the raw map, and add a foreign-checkpoint test where the native parsed timestamp diverges from the UTC reparse.
| /// `timestamp_timezone` must be an IANA time zone identifier recognized by Kernel or a | ||
| /// normalized fixed offset in `+HH:MM` or `-HH:MM` form. An explicit offset in a partition | ||
| /// value takes precedence. This option does not affect `TIMESTAMP_NTZ`. It applies to typed | ||
| /// scan metadata, final partition predicate evaluation, and partition-column row transforms |
There was a problem hiding this comment.
Blocker2 This doc claims with_timestamp_timezone applies to typed scan metadata and final partition predicate evaluation, but the declarative-plans path (kernel/src/scan/scan_plan.rs, feature="declarative-plans", reachable via the public Scan::declarative_metadata_scan_plan) never threads self.partition_values.timestamp_timezone: it reconstructs partition values with MapToStructOptions::default() (UTC) and includes every partition field in stats_skipping_predicate with no zoned-TIMESTAMP deferral. Combining with_timestamp_timezone with the declarative path emits UTC-parsed values that diverge from the non-declarative scan (confirmed by the disprove gate).
Raised by: maintainer-claude-reviewer.
Suggested fix: thread the timezone through the declarative parsing and pruning path to match the non-declarative scan, or if deferred to a later stack PR, make declarative_metadata_scan_plan reject a configured timezone and scope this doc to exclude declarative plans.
| ) -> DeltaResult<Scalar> { | ||
| match (raw, data_type.as_primitive_opt()) { | ||
| (Some(v), Some(primitive)) if v.is_empty() => Ok(primitive | ||
| (Some(value), Some(PrimitiveType::Timestamp)) if !value.is_empty() => timestamp_timezone |
There was a problem hiding this comment.
Nit1 Two parsers now turn the same raw partition string into a typed value: parse_partition_value_raw here and parse_partition_scalar in the MAP_TO_STRUCT UDF (evaluate_expression.rs). This PR shared only the TIMESTAMP slice; DATE and TIMESTAMP_NTZ still parse through different code in each path. Dropping the native-column read makes both parsers authoritative over commits and checkpoints, so the MapToStructOptions invariant that materialization and pruning cannot interpret a value differently is now enforced by hand across two modules.
Raised by: architecture-reviewer.
Suggested fix: route both paths through one per-value parse_partition_scalar(prim, raw, tz) so the UDF loop and transform share a single parser, extending the seam this PR created for timestamps.
| fn parse_timestamp(raw: &str, timezone: TimestampTimezone) -> Option<i64> { | ||
| if raw | ||
| .split_once('.') | ||
| .is_some_and(|(_, suffix)| suffix.bytes().take_while(u8::is_ascii_digit).count() > 6) |
There was a problem hiding this comment.
Nit2 Rejecting fractions with more than 6 digits (previously truncated to microseconds) and offsets beyond the 18-hour bound (previously normalized) are intentional, protocol-defensible strictness changes, but they are observable: a table with sub-microsecond partition strings that used to read now errors.
Raised by: maintainer-codex-reviewer, maintainer-claude-reviewer, delta-protocol-reviewer.
Suggested fix: note the stricter partition-timestamp parsing in the PR description or changelog for downstream connectors relying on the old lenient behavior.
2 participants
Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
Offset-less
TIMESTAMPpartition values need to be interpreted in the configured timezone. This PR addsPartitionValuesOptions::with_timestamp_timezoneand uses it when full snapshot scans convert rawpartitionValuesinto typed scan metadata, final predicate inputs, RowTransforms, and materialized partition columns.Native checkpoint pruning remains enabled for data statistics and timezone-independent partition fields. When a timezone is configured, zoned
TIMESTAMPpartition predicates are deferred until raw partition strings have been reparsed, preventing checkpoint values produced under UTC semantics from incorrectly dropping files. The timezone defaults to UTC and accepts IANA identifiers or canonical+HH:MMand-HH:MMoffsets. An explicit offset in a stored ISO 8601 value takes precedence,TIMESTAMP_NTZremains timezone-independent, and incremental scans and CDF preserve their existing raw-map behavior. Invalid configured timezones fail when the scan is built.How to review
Start with
PartitionValuesOptionsandScanBuilder::build, then follow the option through log replay, final partition filtering, RowTransforms, and scan execution.The incremental diff is primarily tests, with production changes focused on option propagation, safe checkpoint pruning, and partition-value reparsing.
This PR affects the following public APIs
Adds
PartitionValuesOptions::with_timestamp_timezone.How was this change tested?
Unit and integration tests cover JSON and checkpoint scans, safe partition skipping, column mapping, RowTransforms, execution, invalid configuration and value handling, and unchanged incremental-scan behavior.