feat!: support map-to-struct options in engine adapters - #3296
Conversation
github-actions
Bot
added
the
breaking-change
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
==========================================
+ Coverage 90.32% 90.35% +0.03%
==========================================
Files 250 251 +1
Lines 89038 89441 +403
Branches 89038 89441 +403
==========================================
+ Hits 80422 80816 +394
Misses 5682 5682
- Partials 2934 2943 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 13 · ☑️ 2 · 🚧 0 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
![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 MapToStructOptions (a reader timestamp timezone) through the kernel evaluator, the FFI in both directions, protobuf, the expression transform, and the DataFusion executor. The timezone parser (fixed offsets, IANA names, DST gap/overlap resolution) is careful and well tested, and the configured DataFusion path delegates to the kernel Arrow evaluator so its results match kernel semantics. One blocking issue on UDF identity, plus a few non-blocking notes.
Blocking issues
Blocker1 - datafusion-executor/src/expression.rs around line 480 (the impl Hash for KernelMapToStructUdf, and the impl ScalarUDFImpl for KernelMapToStructUdf below it; the pre-existing ParseJsonUdf shares the flaw).
KernelMapToStructUdf derives PartialEq/Eq and hand-implements Hash over output_schema + options, but its ScalarUDFImpl overrides neither equals() nor hash_value(). On DataFusion 54.0.0 the default equals/hash_value compare only name() and signature(). Every instance reports the constant name "kernel_map_to_struct" and Signature::any(1, Immutable), so the concrete PartialEq/Hash are never consulted by the optimizer. Two calls over the same map argument with the same output schema but different configured timezones therefore compare equal, and common-subexpression elimination can collapse them into one, returning the wrong timestamps for one of the fields. A single projection can contain two independently configured parses of the same partition map, so this is reachable.
Raised by: maintainer-claude-reviewer, test-coverage-reviewer (confirmed by disprove-reviewer).
Suggested fix: override equals and hash_value in ScalarUDFImpl for both KernelMapToStructUdf and ParseJsonUdf, downcasting to the concrete type and delegating to the existing PartialEq/Hash so output_schema (and options for map-to-struct) participate in identity. For example:
fn equals(&self, other: &dyn ScalarUDFImpl) -> bool {
other.as_any().downcast_ref::<Self>() == Some(self)
}
fn hash_value(&self) -> u64 {
let mut hasher = std::collections::hash_map::DefaultHasher::new();
self.hash(&mut hasher);
hasher.finish()
}Non-blocking notes
Nit1 - kernel/src/checkpoint/checkpoint_transform.rs around line 215 (build_partition_values_parsed_expr).
Checkpoint reconstruction and the kernel-native scan paths hardcode MapToStructOptions::default() (UTC), while only the engine-facing FFI/proto/DataFusion paths can carry a configured timezone. Pinning UTC into the shared checkpoint artifact is the right deterministic choice, but once an engine configures a non-UTC zone, checkpoint-sourced partitionValues_parsed (UTC) and JSON-commit reconstructions (configured zone) can yield different instants for the same offset-less partition value within one scan. Worth confirming this is intended and documenting the divergence window here and at the log_replay coalesce.
Raised by: delta-protocol-reviewer.
Suggested fix: add a comment describing the cross-path interpretation gap, or constrain the configured timezone to tables without checkpoint-materialized partitionValues_parsed.
Nit2 - kernel/src/engine/arrow_expression/evaluate_expression.rs around line 3001 (#[case::embedded_iana_timezone]).
The case feeds "2024-01-15 12:30:45 America/New_York" and expects the embedded named zone to win over the configured Europe/Berlin. Arrow's string_to_datetime parses Z and numeric offsets but is not documented to parse trailing IANA names, so it is unclear whether this test exercises the named-zone path it describes or passes for another reason. Confirm the assertion reflects real parsing rather than incidental behavior.
Raised by: delta-protocol-reviewer.
Suggested fix: verify what string_to_datetime does with a trailing IANA name; correct or drop the case if it does not parse the named zone, and describe embedded-offset precedence as a read-path leniency rather than a protocol rule.
Nit3 - datafusion-executor/src/expression.rs around line 426.
The primitive-only field check now exists twice with the same error string: inline in lower_default_map_to_struct and again in validate_map_to_struct_target.
Raised by: maintainer-claude-reviewer, architecture-reviewer.
Suggested fix: call validate_map_to_struct_target(target)? once before the default/configured split so the check and its message live in one place.
Nit4 - kernel/src/engine/arrow_expression/evaluate_expression.rs around line 444 (parse_normalized_fixed_offset).
Tests cover the invalid boundary +18:00:01 but not the valid maximum +18:00 / -18:00. An off-by-one turning hours > 18 into hours >= 18 would pass the current suite.
Raised by: test-coverage-reviewer.
Suggested fix: add +18:00 and -18:00 acceptance cases to the existing timezone rstest.
Summary
The change is well structured and unusually well tested for its size, with value-level assertions across the kernel evaluator, both FFI directions, proto, transforms, serde, and the DataFusion executor. The one blocker is that the new (and existing) kernel-backed UDFs do not make their schema and options part of DataFusion identity, which risks incorrect common-subexpression elimination; fixing equals/hash_value resolves it. The remaining notes are a checkpoint-versus-configured-timezone consistency question, a test to verify, and minor cleanups.
Automated review - workflow run
eaaff83 to
deb65cd
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads MapToStructOptions through the kernel expression, the FFI visitors (both directions), the protobuf plan format, and the DataFusion executor. Six reviewers (two maintainer passes, protocol, architecture, test coverage, docs) looked at it. No blocking correctness, protocol, or safety defect was found. The timezone parsing (offset-less interpretation, embedded offset/zone precedence, DST gap and overlap) is correct and well tested, and the configured path preserves kernel-evaluator parity by delegating to it. A few non-blocking notes follow.
Non-blocking notes
Nit1. datafusion-executor/src/expression.rs (RIGHT ~410, KNOWN DIVERGENCES doc block on the default lowering). The default DataFusion path uses arrow's native cast, which cannot parse a value carrying an embedded named timezone (for example 2024-01-15 12:30:45 America/New_York), so it hard-errors. The kernel evaluator now resolves such embedded named zones even on the default UTC path (split_embedded_named_timezone), so it succeeds there. This is a new default-path divergence introduced by this PR and is not in the documented KNOWN DIVERGENCES list (which mentions only duplicate keys, Boolean, and Decimal). It is confined to non-spec-compliant values, so severity is low. Raised by: delta-protocol-reviewer, maintainer-claude-reviewer, maintainer-codex-reviewer. Suggested fix: add embedded-named-timezone to the KNOWN DIVERGENCES note, or route such values through the UDF for parity.
Nit2. datafusion-executor/src/expression.rs (RIGHT ~394, default-lowering rustdoc). The doc says "Numeric and temporal values first pass through nullif(value, '')", but the code applies nullif to every primitive except String and Binary, so Boolean and Void also pass through it. Raised by: docs-reviewer. Suggested fix: reword to "All fields except String and Binary first pass through nullif(value, '')".
Nit3. kernel/src/plans/proto/convert.rs (RIGHT ~327) and datafusion-executor/src/expression.rs (~422). Default detection is options == MapToStructOptions::default(), so an explicit with_timestamp_timezone("UTC") or "+00:00" is semantically the default interpretation but compares unequal. It is then encoded as configured_map_to_struct (tag 14) and rejected by options-unaware executors, and on the DataFusion side takes the UDF path rather than the native lowering, despite identical results. This is consistent with the deliberate absent-vs-configured wire distinction, so it is a documentation choice rather than a defect. Raised by: delta-protocol-reviewer, architecture-reviewer. Suggested fix: document that default() is the compatibility form for UTC, or normalize an explicit UTC/+00:00 to the default at construction.
Nit4. kernel/proto/expressions.proto (RIGHT ~164). "Configured-ness" is encoded in two places: the oneof tag choice (12 vs 14) and the optional options field inside the shared MapToStructExpression message. The shared message permits noncanonical combinations (tag 12 with options, tag 14 without), and the invariant is maintained only by hand in the encoder. Once tag 12 ships carrying an optional options field, that wire shape is fixed. No decoder in this repo mishandles it, so this is a schema-modeling preference, not a demonstrated failure. Raised by: architecture-reviewer. Suggested fix: consider giving tag 14 its own message with a non-optional options and leaving tag 12's message unchanged, so the tag and payload cannot disagree.
Summary
The parsing semantics and the tag-12/tag-14 forward-compat split are sound, and coverage of the new logic (timezone families, both FFI directions, proto split and legacy rejection, transform preservation, default vs configured execution) is thorough. There are no blocking issues; the notes above are documentation and design-clarity suggestions.
Automated review - workflow run
deb65cd to
e091854
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads MapToStructOptions through the map-to-struct expression, both FFI directions, the proto wire format, and the DataFusion executor. I reviewed correctness, protocol compliance, the FFI boundary, tests, and docs. No blocking issues found. The KernelMapToStructUdf identity split (deriving Eq over all fields while hashing only output_schema + options) was checked and is sound: the hashed fields are a subset of the compared fields, so a == b implies equal hashes, and the two extra fields are a deterministic function of the schema plus a constant signature. The proto backward-compatibility scheme (routing configured semantics to a distinct tag 14 so legacy executors reject them, keeping default options on tag 12) is correct and has a decode test pinning it. Timezone parsing, DST handling, fixed-offset bounds, and the FFI UTF-8 rejection path are all covered by tests.
Non-blocking notes
Nit1: kernel/src/engine/arrow_expression/evaluate_expression.rs (Timestamp arm of parse_partition_scalar, around line 964) with kernel/src/timestamp_timezone.rs. The default (no timezone) path now routes through parse_timestamp_with_timezone / split_embedded_named_timezone, so a default MapToStruct accepts a value with a trailing named zone such as 2024-06-15 09:30:00 America/New_York, which the previous string_to_datetime(&Utc, raw) rejected with a ParseError. This default path feeds the checkpoint, data-skipping, log-replay, and scan-plan reconstructions. The broadening is documented in the MapToStructExpression TIMESTAMP contract and the parsing branch is tested with a configured timezone, but there is no regression test pinning the default-options plus trailing-named-zone case. Raised by: maintainer-claude-reviewer. Suggested fix: add a default-options test case asserting the accepted (or rejected) behavior for a trailing named zone so the semantics are pinned.
Nit2: datafusion-executor/src/expression.rs line 424 and kernel/src/plans/proto/convert.rs. The "options are the legacy default" predicate is inlined as map_to_struct.options == MapToStructOptions::default() in two places while MapToStructOptions::is_default() already exists (used only by skip_serializing_if). Keeping three independent encodings of the same distinction risks divergence when a second option field is added, since the serde-skip rule and the proto-tag rule must agree. Raised by: architecture-reviewer, maintainer-claude-reviewer. Suggested fix: make is_default (or a selects_legacy_representation) public and have the datafusion and proto sites call it.
Nit3: kernel/src/lib.rs line 119. The new timestamp_timezone module sits at the crate root with a bespoke #[cfg(all(feature = "arrow-expression", feature = "default-engine-base"))] gate that byte-for-byte duplicates the gate on arrow_expression, even though its only consumer is the arrow evaluator and the module is private. Raised by: architecture-reviewer. Suggested fix: move it under kernel/src/engine/arrow_expression/ so it inherits the parent module gate and lives next to its caller.
Summary
The change is well structured and well tested, and the two headline design questions (UDF identity and proto wire compatibility) both hold up under scrutiny. No blocking defects. The notes above are maintainability and coverage refinements. Protocol interpretation of a reader-supplied timezone as a proxy for the writer's zone, and defaulting to UTC when unconfigured, are honest approximations of an unrecoverable value and are documented as such.
Automated review - workflow run
e091854 to
ac1ebc3
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads MapToStructOptions through the engine adapters cleanly. The default UTC path keeps DataFusion's native named_struct lowering, configured options route through a kernel-backed UDF that reuses kernel's Arrow evaluator, and the proto layer uses a separate ConfiguredMapToStruct tag so legacy decoders reject configured semantics instead of silently reading them as UTC. Tests are thorough, with value-level assertions for DST overlap/gap, fixed-offset bounds, embedded-timezone precedence, both FFI directions, and proto legacy rejection.
No blocking issues.
Two candidate concerns were checked and dropped: the UDF's use of args.number_rows follows the standard DataFusion pattern (arrays are preserved by into_array, number_rows only drives scalar expansion), and the UDF identity does include options in both Eq and Hash with DataFusion 54 delegating equals/hash_value to the impl, so differently-configured expressions are not collapsed by common-subexpression elimination.
Non-blocking notes
Nit1: kernel/src/timestamp_timezone.rs / kernel/src/lib.rs:118. The new module is a crate-root sibling but depends entirely on arrow, carries the arrow-expression + default-engine-base gate, and has one caller inside engine/arrow_expression. Its only constructor, try_from_options(&MapToStructOptions), also reaches up into the expressions layer and bakes in MapToStruct's "absence means UTC" policy, so a second caller in the stacked parsing PR cannot reuse it without fabricating a MapToStructOptions. Raised by: architecture-reviewer. Suggested fix: move the module under engine/arrow_expression/ and give it a value-typed constructor (for example TimestampTimezone::parse(&str)), leaving the UTC-default policy at the expression layer.
Nit2: kernel/src/engine/arrow_expression/evaluate_expression.rs:964. Routing the TIMESTAMP arm through timestamp_timezone.parse_timestamp runs the embedded-named-timezone handling on the default path too, so a non-spec value like 2024-01-15 12:30:45 America/New_York now parses even with default options, where the prior string_to_datetime(&Utc, raw) returned a ParseError. This is confirmed by the embedded_iana_timezone_with_default_options test. Spec-compliant writers never emit this form, but it widens what the default path accepts rather than propagating a parse error. Raised by: delta-protocol-reviewer. Suggested fix: confirm this widening of the default interpretation is intended; if not, restrict embedded-named-timezone handling to the configured path.
Nit3: kernel/src/expressions/mod.rs:977. Making options a required argument on map_to_struct churns every in-crate call site (checkpoint_transform, log_replay, scan_plan, data_skipping), all of which pass MapToStructOptions::default(); only the engine-to-kernel FFI reconstruction supplies a real value. Raised by: architecture-reviewer. Suggested fix: consider keeping map_to_struct(map_expr) defaulting the options and adding map_to_struct_with_options(map_expr, options) for the one configured caller, which is also non-breaking. This is a judgment call; the PR states the break is intentional.
Nit4: kernel/src/scan/scan_plan.rs:495 (also kernel/src/scan/log_replay.rs and kernel/src/checkpoint/checkpoint_transform.rs). The coalesce([partitionValues_parsed, map_to_struct(...)]) prefers the native/checkpoint parsed column over reconstruction. Today every path uses default UTC so there is no divergence, but once the stacked scan-wiring PR threads a non-UTC reader timezone into the reconstruction, a UTC-baked partitionValues_parsed from a checkpoint would win the coalesce and silently override the configured timezone, producing different results for checkpoint-covered vs JSON-commit files. Raised by: delta-protocol-reviewer. Suggested fix: resolve the persistence/precedence policy in the follow-up PR, so a UTC-parsed column is not preferred when a non-default timezone is configured.
Coverage note: all six reviewers and the disprove gate completed.
Automated review - workflow run
ac1ebc3 to
b368d4b
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The timezone-resolution module is careful and well tested (DST overlap and gap, the Pacific/Apia full-day skip, fixed-offset limits, invalid input and invalid UTF-8 rejection), the configured DataFusion path delegates to the kernel Arrow evaluator so parsing stays in sync, and the distinct configured_map_to_struct proto tag correctly makes configured semantics opaque to options-unaware executors. The FFI break to a single visit_map_to_struct signature is justified for a source-level vtable. A DataFusion CSE-identity concern was investigated and cleared: DataFusion 54's default ScalarUDFImpl::equals/hash_value delegate to the concrete type, so KernelMapToStructUdf hashing schema and options is sufficient.
Non-blocking notes
Nit1: kernel/src/engine/arrow_expression/evaluate_expression.rs (RIGHT ~393)
Default MapToStructOptions now routes the timestamp arm through the new TimestampTimezone path, so a value like 2024-01-15 12:30:45 America/New_York parses under default options where the prior string_to_datetime(&Utc, raw) path would have hard-errored. This also reaches checkpoint reconstruction (build_partition_values_parsed_expr) and diverges from the DataFusion default lowering, which still rejects a trailing named zone. The change is confined to non-spec-compliant partition values and is documented in the KNOWN DIVERGENCES list, so it is not a defect, but it is a change to the default MapToStruct contract worth calling out in the PR's public-API note.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer
Suggested fix: note in the PR description that the default path is now strictly more permissive than before (and than the DataFusion default lowering) for embedded named timezones on malformed input.
Nit2: datafusion-executor/src/expression.rs (RIGHT ~424)
MapToStructOptions::is_default() is the selector for three unrelated decisions: proto tag choice (convert.rs), the native-vs-UDF lowering branch here, and serde skip_serializing_if. That is coherent for the single current option, but a second option that the native path could express or that old executors could safely ignore would be forced onto the kernel UDF and rejected by legacy executors unnecessarily.
Raised by: architecture-reviewer
Suggested fix: keep is_default() for serde only, and give the two mechanism layers intent-revealing predicates (for example native_lowerable() for the lowering branch and requires_new_executor() for the proto tag) so each layer decides per option rather than on whole-bag emptiness.
Nit3: kernel/src/engine/arrow_expression/timestamp_timezone.rs (RIGHT ~128)
split_embedded_named_timezone locates the embedded-timezone separator with rfind(char::is_whitespace) and trim(), so it accepts any whitespace (tab, newline) rather than a single ASCII space. This only broadens acceptance of already non-canonical values and yields the same instant, so it has no correctness impact, but tightening it keeps the parser closer to the space-delimited canonical forms.
Raised by: maintainer-codex-reviewer
Suggested fix: restrict the separator to a single ASCII space (or reject \t/\n/\r) on this path if strictness is preferred.
Summary
This change is protocol-compliant and, on the timestamp path, more spec-aligned than the code it replaces, with strong test coverage across parsing, both FFI directions, and the DataFusion default and configured paths. The only actionable items are non-blocking: a documentation call-out for the default-path behavior change, an optional refactor of the is_default() coupling before a second option lands, and an optional separator tightening.
Automated review - workflow run
b368d4b to
9b41d0c
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This change threads an optional reader timezone through the MapToStruct expression end to end: the kernel options type, the Arrow evaluator's timestamp parsing, the DataFusion executor (native lowering for default options, kernel-backed UDF for configured ones), the proto wire format, and both FFI visitor directions. The reviewers found no blocking defects. The timezone parser, DST gap/overlap policy, fixed-offset bounds, embedded-timezone precedence, proto backward-compatibility split, and FFI plumbing are all well tested and behave as documented.
No blocking issues.
Non-blocking notes
Nit1: datafusion-executor/src/expression.rs (default lowering path, around the options.is_default() branch). For default options the native named_struct/cast lowering can disagree with kernel's Arrow evaluator on timestamp forms that kernel deliberately accepts but the native cast does not, for example a trailing named timezone (2024-01-15 12:30:45 America/New_York). The kernel test embedded_iana_timezone_with_default_options shows the evaluator resolving that input, while the native path is documented as not accepting it, so the two executors can produce different results for the same default expression. The divergence is confined to non-spec-compliant partition values, so this is not a correctness problem for spec-compliant tables, but it is a conscious cross-executor tradeoff worth signing off on rather than leaving implicit in the growing KNOWN DIVERGENCES list.
Raised by: architecture-reviewer, maintainer-codex-reviewer
Suggested fix: if exact parity matters, gate the native fast path on the target-type subset where DataFusion's cast is bit-identical to kernel (string, binary, integers) and route timestamp, decimal, and boolean through the UDF regardless of options; otherwise keep the current split and note the accepted divergence explicitly.
Nit2: kernel/src/engine/arrow_expression/timestamp_timezone.rs (fixed-offset bound check). The hours == 18 && (minutes != 0 || seconds != 0) rejection only exercises the seconds arm in tests (+18:00:01); the minutes != 0 half (+18:30) is never taken, and the Z/z branch of the embedded offset-plus-named-timezone conflict check is also untested. A regression that dropped either sub-check would go unnoticed.
Raised by: test-coverage-reviewer
Suggested fix: add rstest cases for +18:30/-18:30 to rejects_invalid_fixed_offsets, and a ...Z America/New_York case asserting the conflict error.
Nit3: kernel/src/engine/arrow_expression/evaluate_expression.rs (parse_partition_scalar doc). The comment says timestamp_timezone applies to offset-less TIMESTAMP values, but a value carrying an embedded named timezone is also unaffected because the embedded zone takes precedence. "Offset-less" alone slightly overstates when the reader timezone applies.
Raised by: docs-reviewer
Suggested fix: reword to "applies only to TIMESTAMP values that carry neither an offset nor an embedded named timezone; it does not affect DATE or TIMESTAMP_NTZ."
Automated review - workflow run
9b41d0c to
8e9820a
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR wires MapToStructOptions through the engine adapters so a configured reader timezone is preserved and executed rather than rejected. The design is coherent: default UTC options keep the native DataFusion named_struct/cast lowering, configured options go through a kernel-backed scalar UDF whose identity folds in both the output schema and the options, and the proto layer routes configured semantics to a distinct fail-closed tag. The timezone parsing has a strong test matrix (DST overlap/gap, embedded IANA names, fixed offsets, offset limits, invalid inputs). No blocking issues.
Non-blocking notes
Nit1 — kernel/src/expressions/mod.rs (around line 735)
The MapToStructExpression doc still says the reference evaluator implements the rules with PrimitiveType::parse_scalar and "equivalent Arrow parsers for dates and timestamps". After this change, TIMESTAMP no longer uses an Arrow parser; it goes through the chrono/chrono-tz TimestampTimezone::parse_timestamp. Only DATE (Date32Type::parse) and TIMESTAMP_NTZ (string_to_datetime) remain Arrow-backed, so the sentence is now inaccurate for TIMESTAMP.
Raised by: docs-reviewer
Suggested fix: reword to distinguish the kernel timezone-aware TIMESTAMP parser from the Arrow-backed DATE and TIMESTAMP_NTZ paths.
Nit2 — ffi/src/test_ffi.rs (line 163)
The configured with_timestamp_timezone("America/Los_Angeles") expression only appears in get_testing_kernel_expression, whose C fixture sets validate_roundtrip = false. The only roundtrip-enabled fixture (get_simple_testing_kernel_expression) uses default options, so the engine-to-kernel C reconstruction of a configured timezone (the strlen/allocate_string/free and visit_expression_map_to_struct Some branch) is compiled but never executed end to end. Rust unit tests cover both visitor directions, so this is a gap in the C integration path only.
Raised by: test-coverage-reviewer
Suggested fix: add a configured-timezone map-to-struct to a roundtrip-enabled fixture and assert equality after the full C roundtrip.
Nit3 — datafusion-executor/src/expression.rs (line 408)
The KNOWN DIVERGENCES block lists only a trailing named timezone as accepted by the kernel parser but not the native DataFusion cast. The kernel parser's offset-less timestamp handling also accepts other lenient forms (short/compact numeric offsets, lowercase t, compact clock), which the native cast may reject. Under default options, kernel-backed data skipping and DataFusion materialization could then disagree on those non-spec values. This is confined to malformed inputs, and reviewers did not confirm each specific native-cast rejection, so treat it as a judgment call.
Raised by: delta-protocol-reviewer, maintainer-claude-reviewer
Suggested fix: either confirm each additional divergence with a test and enumerate them, or note that the listed case is illustrative rather than exhaustive.
Summary
The change is well structured and well tested, and I found no correctness, protocol, or safety defects present in the diff. The proto fail-closed tag split, the UDF identity that includes options, and the FFI signature change in both directions are all handled correctly. The remaining items are documentation accuracy and a C-level test gap, none of which block merge.
Automated review - workflow run
f7839ab to
df6261b
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR threads MapToStructOptions (reader timestamp timezone) through the expression tree, both FFI directions, the proto wire format, and the two DataFusion adapter lowerings. I reviewed protocol compliance, the Rust changes (UDF identity, the new unsafe FFI constructor, the timezone parser), architecture, tests, and docs. No blocking issues. The wire/ABI compatibility work is careful: default options skip serialization, the proto tag is preserved and tested, and the breaking FFI signature change is correctly flagged with feat!. The DST/offset/embedded-timezone test matrix is thorough, and the identity and both-direction FFI paths are covered.
Three concerns worth escalating (UDF identity/CSE collision, a default-path timestamp parser regression, and a pruning-vs-materialization timezone mismatch) were checked against the pinned code and all resolved as non-issues: DataFusion 54 delegates ScalarUDF equality/hashing to the implementation, so differing options stay distinct; the new parser covers the Arrow forms the default path previously accepted; and every kernel-internal map_to_struct call site uses MapToStructOptions::default(), with no scan-level API in this PR able to configure materialization independently of pruning.
Non-blocking notes:
Nit1: kernel/src/lib.rs:118 (also kernel/Cargo.toml feature edit). The new timestamp_timezone module is a pure std/chrono/chrono-tz parser placed at crate root as a pub(crate) concern, yet gated on arrow-expression and consumed by exactly one module (engine/arrow_expression/evaluate_expression.rs). The placement and the gate pull in opposite directions, and the next non-arrow consumer (for example the proto path that already round-trips the timezone string) would have to re-cut the gate.
Raised by: architecture-reviewer.
Suggested fix: pick one altitude, either move the module beside its sole caller under engine/arrow_expression/ so it inherits that gating, or give chrono-tz its own narrow feature (for example partition-timezone) that arrow-expression enables and keep the module ungated on arrow.
Nit2: datafusion-executor/src/expression.rs:1213 (map_to_struct_udf_identity_includes_options). The test only asserts inequality across two timezones with the same output schema. Because Hash is hand-written (schema + options) while Eq is derived (also return_type + signature), a regression that drops a field from hash_output_schema or the options hash would not be caught, and the equal-config case that CSE relies on to actually dedupe is unasserted.
Raised by: test-coverage-reviewer.
Suggested fix: add assertions that same schema + same options compares equal, and that same options + different output schema compares unequal, for KernelMapToStructUdf.
Nit3: datafusion-executor/src/expression.rs:408. The added KNOWN DIVERGENCE bullet ("a timestamp with a trailing named timezone is accepted by the kernel parser but not by the native DataFusion cast") sits under the intro that scopes divergences to values "spec-compliant writers never emit." Kernel's default path deliberately accepts this form (see embedded_iana_timezone_with_default_options), so on the default lowering the kernel evaluator parses such a value while the native DataFusion cast hard-errors. That gap can surface on real legacy tables, not only theoretical inputs.
Raised by: docs-reviewer, delta-protocol-reviewer, test-coverage-reviewer.
Suggested fix: soften the intro wording (for example "malformed, non-canonical, or non-spec values") or note explicitly that this bullet can appear on historically-accepted tables, and consider a DataFusion-executor test pinning the default path's actual outcome on a trailing-named-timezone value.
Nit4: ffi/src/expressions/kernel_visitor.rs:697. The # Safety section for visit_expression_map_to_struct states the slice must "point to its declared number of initialized bytes and remain valid for this call" but omits the non-null / valid-pointer requirement that String::try_from_slice relies on. FFI safety docs should state the full contract the callee depends on.
Raised by: maintainer-claude-reviewer, maintainer-codex-reviewer.
Suggested fix: extend the safety note to require a non-null pointer to a valid, readable buffer of exactly len bytes for the duration of the call.
Summary: The change is well-shaped and well-tested, with careful attention to wire and ABI backward compatibility and to timezone parsing semantics. No correctness or protocol defects present in the diff. The notes above are hardening and documentation-accuracy improvements, not merge blockers.
Automated review - workflow run
df6261b to
f5d22bb
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. The change threads MapToStructOptions through the kernel evaluator, engine adapters, FFI (both directions), and the proto/serde wire format cleanly, and it is well covered by tests. The reader-timezone default of UTC matches how delta-spark reads offset-less timestamp partition values, the DST overlap/gap and offset-versus-named-zone rules are handled correctly, and DATE and TIMESTAMP_NTZ are left untouched. Wire compatibility is preserved: default options are omitted from JSON and keep the existing proto tag.
Non-blocking notes:
Nit1: kernel/src/expressions/mod.rs (MapToStructExpression doc, around line 735). The sentence "Kernel's reference evaluator implements these rules with PrimitiveType::parse_scalar and equivalent Arrow parsers for dates and timestamps" is now stale. After this change parse_partition_scalar routes TIMESTAMP through TimestampTimezone::parse_timestamp (chrono/chrono-tz), not an Arrow parser; only DATE (Date32Type::parse) and TIMESTAMP_NTZ (string_to_datetime) still use Arrow parsers.
Raised by: docs-reviewer.
Suggested fix: reword to distinguish the dedicated timezone-aware TIMESTAMP parser from the Arrow parsers used for DATE and TIMESTAMP_NTZ.
Nit2: datafusion-executor/src/expression.rs:424. map_to_struct_to_df_expr uses MapToStructOptions::is_default() to choose native named_struct lowering versus the kernel UDF, but is_default() is also the wire-omission predicate for serde and proto. Today default == UTC == natively lowerable coincide, so this is correct, but the coupling is load-bearing and undocumented: a connector that normalizes its session zone to the literal string "UTC" is not default and silently loses the native fast path, and a future second option would be misclassified for lowering.
Raised by: architecture-reviewer, maintainer-claude-reviewer, maintainer-codex-reviewer.
Suggested fix: branch the adapter on the capability it actually needs (for example map_to_struct.options.timestamp_timezone().is_none()) and keep is_default() as a pure serialization predicate.
Nit3: kernel/src/lib.rs:118. The new timestamp_timezone module is pub(crate) and gated on arrow-expression, with its only consumer in engine/arrow_expression/evaluate_expression.rs, yet it sits at the crate root beside protocol-level modules. This invites future non-arrow code to reach for it and be surprised by the feature gate (and the chrono-tz dependency).
Raised by: architecture-reviewer.
Suggested fix: move it under kernel/src/engine/arrow_expression/ as a submodule of its sole consumer; visibility can stay pub(crate).
Summary: This is a solid, well-tested change with no correctness, protocol, or safety defects found in the diff. A candidate blocker about the KernelMapToStructUdf Hash/Eq asymmetry was checked and dropped: return_type is derived from output_schema and signature is constant, so equality reduces to (output_schema, options) and the hash contract holds. The remaining items are documentation accuracy and maintainability notes. Coverage was full: all six reviewers and the disprove gate completed.
Automated review - workflow run
| 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.
Nit2 is_default() gates the native named_struct lowering versus the kernel UDF, but it is also the wire-omission predicate for serde and proto. Today default == UTC == natively lowerable coincide, so this is correct, but the coupling is load-bearing: a connector that normalizes its session zone to the literal string "UTC" is non-default and silently loses the native fast path, and a future second option would be misclassified for lowering. Raised by: architecture-reviewer, maintainer-claude-reviewer, maintainer-codex-reviewer. Suggested fix: branch the adapter on the capability it needs (e.g. map_to_struct.options.timestamp_timezone().is_none()) and keep is_default() as a pure serialization predicate.
| pub mod table_features; | ||
| pub mod table_properties; | ||
| #[cfg(feature = "arrow-expression")] | ||
| pub(crate) mod timestamp_timezone; |
There was a problem hiding this comment.
Nit3 The new timestamp_timezone module is pub(crate) and gated on arrow-expression, and its only consumer is the arrow expression evaluator, yet it sits at the crate root beside protocol-level modules. That placement invites future non-arrow code to reach for it and hit the feature gate (and the chrono-tz dependency). Raised by: architecture-reviewer. Suggested fix: move it under kernel/src/engine/arrow_expression/ as a submodule of its sole consumer; visibility can stay pub(crate).
d90ec03 to
25f545e
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. This change threads MapToStructOptions through the kernel evaluator, both FFI directions, the proto/serde wire format, and the two DataFusion adapter lowerings. Partition-value parsing semantics are preserved: UTC default, embedded offset/named-zone precedence over the reader zone, DATE and TIMESTAMP_NTZ left untouched, and wire/ABI compatibility kept (default options omitted from JSON, proto tag preserved, the breaking FFI signature correctly labeled feat!). The timezone parser and its DST overlap/gap, fixed-offset, and embedded-zone handling are correct and well tested, and the UDF identity folds options into equality/hash so differently configured expressions are not merged by common-subexpression elimination.
Summary
The change is well shaped and thoroughly tested, with careful wire and ABI compatibility and sound timezone handling. No correctness, protocol, or safety defect is present in the diff. Several maintainability and doc-accuracy points raised in this run (the is_default() coupling between wire omission and native lowering, the timestamp_timezone module placement, the one-sided UDF identity test, and the KNOWN DIVERGENCES doc scoping) duplicate prior AI review notes at this same head SHA with unchanged behavior and are not re-reported; the earlier FFI safety-doc note has been addressed. Coverage was full: all six reviewers and no disprove gate was required since there are no blocking or should-fix findings.
Automated review - workflow run
| } | ||
|
|
||
| impl KernelMapToStructUdf { | ||
| fn try_new(output_schema: KernelSchemaRef, options: MapToStructOptions) -> DeltaResult<Self> { |
There was a problem hiding this comment.
Nit1 KernelMapToStructUdf::try_new is already fallible and validates the output schema, but it does not parse options.timestamp_timezone(). An invalid connector-supplied zone therefore builds a valid logical and physical plan and only errors at row-evaluation time inside invoke_with_args -> evaluate_expression -> TimestampTimezone::try_from_options (see configured_map_to_struct_reports_invalid_timezone, which asserts the error surfaces at physical.evaluate, not at lowering). Timezone validity is a property of the configured options, decided once, but it is decided per-execution. Low severity, and downstream of the timestamp_timezone module being kernel-internal and arrow-gated. Raised by: architecture-reviewer. Suggested fix: validate the zone in try_new so a bad config fails at plan construction rather than mid-scan, exposing a narrow kernel timezone-validation entry point for the executor to call.
25f545e to
e531040
Compare
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues.
This change threads MapToStructOptions (an optional reader timestamp_timezone) through the kernel evaluator, both FFI directions, the proto/serde wire format, and the two DataFusion adapter lowerings. Partition-value timestamp semantics are preserved: UTC by default, embedded offset or named zone taking precedence over the reader zone, and DATE and TIMESTAMP_NTZ untouched. Wire and ABI compatibility hold: default options are omitted from JSON and proto (existing map_expr tag kept), and the breaking FFI callback/constructor change is correctly labeled feat! with a sound safety contract and invalid-UTF-8 rejection. The KernelMapToStructUdf folds output schema and options into its Hash/Eq identity so common-subexpression elimination does not merge differently configured expressions, and the fixed-offset and DST overlap/gap handling in the new timezone parser is correct and well tested.
One candidate finding (UTC aliases like utc/Z being rejected by the parser) was checked and dropped: the option format is documented as canonical IANA identifiers or normalized offsets, no call site supplies such aliases, and rejecting unrecognized strings is intended validation.
The maintainability and doc-accuracy notes surfaced this run (the is_default() coupling between wire omission and native lowering, the crate-root placement of the timestamp_timezone module, execution-time-only timezone validation in the UDF, the one-sided UDF identity test, and the stale Arrow-parser doc sentence) duplicate prior AI review notes at this same head SHA with unchanged behavior and are not re-reported.
Coverage was full: all six reviewers completed and the disprove gate returned a verdict.
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?
This PR enables the engine adapters to preserve and execute the
MapToStructOptionsintroduced by the preceding PR instead of rejecting configured expressions.DataFusion retains its native
named_structand cast lowering for default UTC options. When a reader timezone is configured, it uses a Kernel-backed scalar UDF so timestamp parsing has the same semantics as Kernel's reference Arrow evaluator. The UDF identity includes both the output schema and options, preventing differently configured expressions from being treated as the same common subexpression.Both expression-visitor FFI directions now carry the optional timestamp timezone directly. Kernel-to-engine visitors expose it alongside the child expression, and engine-to-Kernel construction copies it into
MapToStructOptions. This intentionally changes the existing MapToStruct FFI callback and constructor signatures instead of adding a second expression API.This PR affects the following public APIs
The MapToStruct FFI visitor callback and constructor now accept an optional timestamp timezone.
How was this change tested?
Unit and integration tests cover default and configured DataFusion execution, UDF identity, both FFI directions, invalid input handling, and the C expression example.