TASK-35: build the callout struct in feature_names_in_ order - #18
Closed
claude-agent-ahrzb[bot] wants to merge 2 commits into
Closed
TASK-35: build the callout struct in feature_names_in_ order#18claude-agent-ahrzb[bot] wants to merge 2 commits into
claude-agent-ahrzb[bot] wants to merge 2 commits into
Conversation
…r — TASK-35
The authoring path built the named_struct in the user's SQL call order, then
both engines reordered it back to feature_names_in_ -- a scramble we chose,
repaired twice (once in Python, once in Rust). in_schema was derived from the
call order too, so the struct and the declared Exact signature could disagree
whenever the two orders differed. Three of the four blocking bugs on TASK-3
were exactly that desync, and twice a fix introduced the next instance.
Now the struct AND in_schema are both built in feature_names_in_ order. One
order everywhere -- struct, schema, and what the engines feed sklearn -- so the
class is unrepresentable rather than fixed-but-fragile.
The engine-side reorders (_transformer_udf.py, src/expr.rs) are RETAINED on
purpose: hand-authored SQL can still supply a struct in any order, and they
remain its defense (test_struct_field_order_independence_parity covers it).
No user-visible change: the reorder is at build time, so {sc}(income, age)
still equals {sc}(age, income).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
claude-agent-ahrzb Bot
pushed a commit
that referenced
this pull request
Jul 24, 2026
…ing flagged to Wren
…K-35 AC#3
AC#3's second clause: the retained reorders each carry a comment stating WHY
they stay. The existing comments explained WHAT they do; these add the
post-TASK-35 rationale -- the {t}(...) path now emits fitted order so the
reorder is a no-op for it, but hand-authored SQL can supply any struct order
and the reorder is its only defense. Comment-only; no behaviour change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Closing as obsolete: the SQLTransform line this targets has been reset (#54). The DataFusion native engine, the codegen backend, and the batch transform() path are removed; what remains is the confit-served fit-and-serve path. Nothing here is lost — the branch and its commits stay reachable if this work is ever revived. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the call-order vs fitted-order desync bug class unrepresentable. AmirHossein's insight, measured before filing.
The problem — we scramble, then unscramble twice, in two languages
We control step 1, so we chose an order and then spent two independent implementations repairing the mismatch we created.
in_schemawas derived from the call order too, so the struct and the UDF'sExactstruct signature could disagree whenever call order ≠ fitted order.Three of the four blocking bugs found across TASK-3's review rounds were exactly that desync, and twice a fix for one instance introduced the next. That's a design problem, not a coding one.
The change
With one order used everywhere — struct,
in_schema, and what the engines feed sklearn — there is no second order to disagree with. The class stops being fixed-but-fragile and becomes unrepresentable.Scope caveat (AC#3) — the engine-side reorders are RETAINED
Steps 2 and 3 go no-op for the
{sc}(...)authoring path but stay, each with a comment saying why: hand-authored SQL can still supply anamed_structin any order —— so they remain its defense. The win is the authoring path no longer manufacturing mismatches for them to repair, not that the repair disappears.
test_struct_field_order_independence_paritycovers the hand-authored out-of-order case against both engines.Not a semantic change (AC#6)
The reorder is at build time, so
{sc}(income, age)still equals{sc}(age, income). Whether it should — sklearn refuses a reordered DataFrame — is a separate, deliberately-unresolved question, out of scope here and untouched.Acceptance criteria
_named_struct([by_name[f] for f in feat])test_authored_struct_is_built_in_fitted_order_not_call_order(new)_probe(obj, feat, table)_transformer_udf.py,src/expr.rsunchanged, comments presenttest_struct_field_order_independence_paritytest_struct_field_order_independence_parity(both engines)test_call_order_does_not_change_the_result(new)Verification
src/*.rschange — the retained Rust reorder was already correct.'income' != 'age').🤖 Generated with Claude Code