Skip to content

fix: preserve supplied VALUES field metadata [skip ci] - #174

Merged
osipovartem merged 1 commit into
df55-physical-udf-instancefrom
fix/values-schema-metadata
Nov 1, 2026
Merged

osipovartem merged 1 commit into
df55-physical-udf-instancefrom
fix/values-schema-metadata

Conversation

@osipovartem

Copy link
Copy Markdown
Collaborator

Which issue does this PR close?

Narrow runtime-branch backport of metadata preservation already present upstream in Apache DataFusion apache#21714 (commit 49c67c296b9b43d98768fcd1592c6ee1bcde4dfb). No duplicate upstream contribution is needed.

Rationale for this change

LogicalPlanBuilder::values_with_schema currently drops the supplied fields' metadata. This loses logical annotations such as canonical timestamp-with-offset and UUID even though the requested storage type and nullability remain intact. Downstream validation can consequently see a generic struct rather than its declared extension type.

What changes are included in this PR?

  • Preserve annotated target fields with the existing native FieldMetadata::new_from_field API.
  • Retain the original ValuesFields::push path for empty metadata. Metadata work is per annotated column, not per row.
  • Keep generated column names, coercion checks, datatypes and nullability unchanged.
  • Add canonical timestamp-with-offset, non-null/NULL, plain-struct negative-control, custom metadata and integer-coercion coverage.
  • Strengthen normal/prepared UUID INSERT checks before and after parameter binding. Update only four EXPLAIN projection lines: the metadata-only cast is redundant once VALUES retains the annotation.

Are these changes tested?

  • Genuine baseline metadata-loss regression reproduced before the fix.
  • Full required extended workspace command passed offline on pinned Rust 1.97.0: 10,748 passing Rust tests, zero failures and 10 ignores across 72 test summaries; the custom SQLLogicTest runner also exited zero.
  • cargo clippy --all-targets --all-features -- -D warnings passed with the locked offline CI profile.
  • The complete dev/rust_lint.sh suite passed, including formatting, strict workspace clippy, TOML, licenses, spelling, Markdown and strict rustdoc. The unrelated preexisting configuration-table formatting repair is isolated in chore: format runtime configuration table [skip ci] #173.
  • Final independent read-only review approved the exact two-file patch and checked the raw cost-screen results. Four alternating fresh-process pairs used the same locked harness and pinned guest CPU, with five warmups and twenty samples per case. All eight processes passed validation and retained stable identities.
Builder case, eight columns Baseline median Candidate median Cost change
Empty metadata, one row 3.096 us 3.210 us +3.69%
Empty metadata, 4096 rows 6.898 ms 6.910 ms +0.17%
Four metadata keys per field, one row 3.131 us 17.098 us +446.08%, cost only
Four metadata keys per field, 4096 rows 6.814 ms 6.970 ms +2.29%, cost only

These are medians of per-process medians in the CI profile, not release or end-to-end SQL/HTTP/Iceberg/SPCS measurements. Both equivalent-output empty controls meet the predeclared 5% median budget. Individual adverse pairs remain visible: the one-row empty control ranges from +0.76% to +65.69%, so no zero-overhead or statistical non-regression claim is made. Annotated baseline output is incorrect; those rows measure the cost of retaining requested metadata, not an equivalent-output speedup. The added annotation work is proportional to annotated columns/metadata bytes, not VALUES row count.

Are there any user-facing changes?

Supplied field annotations now survive VALUES construction as requested. There is no public API signature change. This backport alone does not establish complete Rustice timestamp alias, storage, CTAS or wire compatibility; those require separate downstream qualification.

@osipovartem
osipovartem merged commit ab14407 into df55-physical-udf-instance Nov 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant