Skip to content

fix: preserve unchanged VALUES schemas during binding [skip ci] - #175

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

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

Conversation

@osipovartem

Copy link
Copy Markdown
Collaborator

Which issue does this PR close?

Follow-up to #174. Issues are disabled in this fork; the reproducible bug and
regression evidence are described below. Base: df55-physical-udf-instance at
ab14407b730762126717039a77eed0a91ef75420.

Rationale for this change

VALUES can initially retain a canonical Arrow logical field, then lose it during
an unrelated with_param_values call. Parameter replacement unconditionally
re-infers VALUES from its expressions, even if nothing was substituted. With
Parquet child field IDs, a type-only row coercion is not a substitute for the
supplied logical field: the extension name and declared timestamp precision
disappear. Rustice subsequently rejects that untagged struct at its existing
schema-aware gate.

What changes are included in this PR?

  • Carry native child/subquery/expression change flags through the existing
    parameter traversal. Keep the full original VALUES schema when unchanged.
  • Continue re-inference for actual bindings and changed subqueries. Preserve
    native traversal order, recursion protection, parent schema recomputation,
    PREPARE verification, and DML/UNION validation.
  • Add four regressions for canonical TimestampWithOffset with child IDs and
    precision 9 versus row casts at 6; NULL and plain-struct controls; empty and
    unrelated maps; PREPARE; projections; direct and subquery substitutions.

There is no additional whole-plan traversal, blind metadata overlay, per-row
value conversion, type-gate relaxation, or new dependency.

Are these changes tested?

Local Rust 1.97.0, offline/locked qualification:

  • Genuine red: both new no-op preservation regressions fail against the original
    production implementation. The corrected source is restored byte-for-byte.
  • Full datafusion-expr library: 268 passed.
  • Required extended workspace command: exit 0; 72 Rust harness summaries,
    10,752 passed, 0 failed, 10 ignored. The custom SQLLogicTest runner also exits 0.
  • Full dev/rust_lint.sh: exit 0, including strict workspace Clippy, formatting,
    TOML, licenses, typos, pinned Prettier and rustdoc.
  • Separate cargo clippy --all-targets --all-features --profile ci --offline --locked -- -D warnings: exit 0.
  • Independent read-only agent review; confirmed feedback addressed before merge.

Focused performance screen

Four alternating fresh-process pairs, one guest CPU, identical frozen harness,
aligned dependency locks, recursive protection enabled, five warmups and twenty
samples per case/process. Source cloning and result validation/drop are outside
timing. Every timed output is checked for typed values, dimensions, names,
nullability and the declared metadata expectation.

Parameter-replacement case, eight columns Before After Comparison
Plain VALUES, one row 9.250 us 2.475 us -73.24%
Plain VALUES, 4096 rows 24.331 ms 8.112 ms -66.66%
Actual binding, one row 14.296 us 14.244 us -0.36%
Actual binding, 4096 rows 47.525 ms 46.444 ms -2.28%
Annotated VALUES, one row 9.672 us 2.538 us Corrected-output cost only
Annotated VALUES, 4096 rows 24.534 ms 8.098 ms Corrected-output cost only

These are medians of per-process medians in a local unoptimized CI profile, not
SQL, release, Iceberg, HTTP or SPCS latency. All four equivalent-output medians
meet the predeclared 5% regression budget. The one-row binding pairs include
an adverse +48.79% sample; it is retained, not discarded, so this does not prove
zero overhead or statistical non-regression. Annotated baseline output is wrong
and receives no equivalent-output speedup claim.

The valid cohort contains 960 timed samples and 240 warmups across eight
zero-exit processes. An earlier cohort stopped at a validator error: it did not
recognize DataFusion's required $1 name-preserving alias. Its failed receipt
and binaries are retained separately; it contributes no performance result.
The harness runner's nine negative/control tests pass normally and under
python -O, including timeout receipts with partial output.

Are there any user-facing changes?

Unchanged parameter binding no longer strips supplied VALUES field metadata.
Actual substitutions still determine the resulting schema. Public API signatures
are unchanged. This fork fix does not itself update Rustice's dependency pin or
claim downstream connector qualification.

@osipovartem
osipovartem merged commit 95e77b7 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