peterxcli commented on PR #5407: URL: https://github.com/apache/datafusion-comet/pull/5407#issuecomment-5447547338
Thanks for the structural pass @andygrove — responses per point: **FFI/Arrow-bridge blast radius.** The change is narrower than it looks: arrow-rs's [`TryFrom<&DataType>`](https://github.com/apache/arrow-rs/blob/58.4.0/arrow-schema/src/ffi.rs#L667-L709) already exported struct *children* as full Fields, so nested name/nullability/metadata fidelity is unchanged — only the top-level export changes. Previously the top level crossed with an empty name, no metadata, and `Flags::empty()` (i.e. unconditionally non-nullable); [`TryFrom<&Field>`](https://github.com/apache/arrow-rs/blob/58.4.0/arrow-schema/src/ffi.rs#L798-L816) makes it faithful. What newly crosses is only what our own serde attaches to plan output fields — the `arrow.parquet.variant` marker and `PARQUET:field_id` — because the exported field is the plan's required-schema field, never a file-derived one; the schema adapter maps file fields onto plan fields before export. On the JVM, `Utils.fromArrowField` maps a Struct to Variant only on the exact `ARROW:extension:name = arrow.parq uet.variant` marker (and natively `is_variant_field` uses arrow-rs's canonical extension check, which also validates the storage shape), so a user struct cannot be misclassified. On #5292: it keys off a different metadata constant (`SPARK::calendarInterval::struct`) on child fields, checked in the same `fromArrowField` match, so the two compose; the overlap is a textual merge conflict, and the Field-based export actually helps that PR's metadata survive FFI. **Per-scan Arrow-schema policy.** The scoping is structural rather than by convention: `skip_arrow_schema` is computed per scan from the pruned required schema (`any(is_variant_field)`), carried as a reader-factory field, and applied only under `skip_arrow_schema && !encrypted`; the Decimal256 narrowing lives inside the Variant-only `cast_column/variant` module. A non-Variant scan cannot take the path without changing both explicit gates. I added the comment you suggested at the decision point in `parquet_exec.rs` stating exactly which condition gates it. **Coverage.** The cases you list are covered: typed_value-present/value-absent (fully shredded) in e.g. `test_normalize_shredded_variant_compacts_spark_integer_widths` and the forced-shredding end-to-end tests; value-present/typed_value-absent (unshredded) throughout; both-present reconstruction precedence in the `test_normalize_partially_shredded_*` group; and nested Variant inside struct/array/map asserts an explicit fallback in `variant.sql` while projecting a pruned sibling stays native. The genuinely missing case was an entirely null Variant column end to end — added in `ParquetReadSuite` covering both unshredded and shredded writes (the Rust side already had `test_normalize_variant_skips_empty_children_of_null_parent`). **UTF-16 rewrite.** Variant-only, structurally: the rewrite lives in `cast_column/variant.rs`, a private module reachable only through `normalize_variant_array`, which the schema adapter installs only for marked Variant target fields. It reorders Variant object keys and metadata dictionaries; general string columns and their ordering semantics are untouched anywhere in Comet. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
