TASK-38: fold an unquoted struct-column qualifier - #21
Closed
claude-agent-ahrzb[bot] wants to merge 6 commits into
Closed
TASK-38: fold an unquoted struct-column qualifier#21claude-agent-ahrzb[bot] wants to merge 6 commits into
claude-agent-ahrzb[bot] wants to merge 6 commits into
Conversation
Carries the qualifier as a QualifiedName (value + quoted) so the fold decision can be made where it belongs -- in plan.rs, which is the only place that knows whether a qualifier names a relation or a struct column. Records four measured findings that changed the ticket: the one-line fix breaks 96 tests; expr_build cannot make the decision; unnest_display_name also needs the fold (the ticket omits it); and the relation branch is inverted vs the oracle but out of scope (DRAFT-22, AC#6 discharged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Four tasks. Task 1 is a deliberate pure-refactor split -- QualifiedName threaded through with every consumer taking .value, suite proven byte-identical (572/12) before any behaviour changes -- so a regression is attributable to one commit. Task 2 carries the behaviour change and both mutation checks. Enumerates all five .table consumers with an explicit relation-vs-data classification, since assigning one wrong reintroduces the 96-failure signature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A qualifier is either a relation alias or a struct column, and expr_build
cannot tell which -- only plan.rs knows, once the relation lookup hits or
misses. Carrying it as QualifiedName { value, quoted } preserves the SQL
quoting to that decision point, so each consumer can ask for the form it
needs.
Pure refactor: every consumer takes .value, which is exactly what the old
String held. No behaviour change; suite identical.
SELECT S.x against struct column s raised 'Unknown column: S' on native while DataFusion folded the qualifier like any identifier and returned the value. So TASK-28's folding rule silently did not apply in the struct-qualifier position. The fold happens in plan.rs's struct-column fallback -- the branch reached only once the qualifier has FAILED to resolve as a relation alias -- so relation qualifiers are untouched. Folding earlier, in expr_build, breaks them: our own rewrite emits __THIS__.age, and folding that misses (measured: 96 failures). A quoted qualifier still stays case-exact, pinned by a new test. That test also exposed a PRE-EXISTING codegen bug -- codegen ACCEPTS "S".x where the oracle rejects it (verified on origin/master, before this branch) -- so it carries an xfail_on_codegen(strict) marker and wants its own ticket rather than an inline fix. The planned unnest_display_name fold was DROPPED: resolving unnest(T.s) needs folding on the relation-alias lookup, which is exactly the 96-failure path this design forbids, and both engines fail it identically. That is DRAFT-22's territory, not this ticket's -- the plan was wrong to scope it here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The TASK-28 ceiling note said a struct-column qualifier 'won't fold ... not reachable today' and advised widening the fold to parts[0]. Both are now false: the case is reachable and fixed, and folding parts[0] is exactly the change that breaks relation qualifiers (measured 96 failures). Replaced with why the qualifier is carried unfolded, plus a pointer to DRAFT-22 for the relation half that still diverges -- which is also what blocks unnest(T.s). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ASK-38 Final review caught that commit a5b0059's message was WRONG. It claimed the unnest_display_name fold was dropped; only the TEST was dropped. The fold did ship (plan.rs renders t.folded()) and was running with zero coverage -- mutating it back to .value passed the entire suite. The fold is correct: DataFusion renders the REGISTERED (folded) qualifier into unnest output column names, so a relation registered as `T` yields t.s.x, and native now matches. What was untestable is the case the plan originally chose -- `unnest(T.s) FROM t`, where the qualifier's case differs from the relation -- because that needs relation-alias folding, the path this design forbids. Using a relation registered as `T` exercises the fold directly. Codegen renders the qualifier raw, so it diverges from the oracle here. Its Python is untouched by this branch; pinned xfail_on_codegen(strict), own ticket. Also hardens test_quoted_qualifier_stays_case_exact with match="S" (it accepted ANY error from either engine, so it could have passed for an unrelated reason), and drops the dangling draft-number references from the shipped source comment. 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.
SELECT S.xagainst a struct column namedsraisedUnknown column: Son native, while DataFusion folds the unquoted qualifier like any other identifier and returns the value — so TASK-28's folding rule silently did not apply in the struct-qualifier position.This is TASK-28's AC#5 ceiling, which shipped with a note saying the case was "unreachable today". Struct field access made it reachable; the flag did its job.
Why the one-line fix doesn't work
The ticket framed this as folding the qualifier in
expr_build.rs. Measured:Our own rewrite emits
SELECT __THIS__.age / __STATE__.avg_age … FROM __THIS__ LEFT JOIN __STATE__, so folding at parse turns__THIS__into__this__and every windowed transform on native dies.expr_buildstructurally cannot make the call: a qualifier is either a relation alias or a struct column, and onlyplan.rsknows which — onceresolved.get(t)hits or misses.The fix
The qualifier is carried to that decision point as a type instead of a bare string:
Fold only where the qualifier names data; never where it names a relation.
resolve_column,resolve_column_type,column_qualifier,validate_expr's alias lookup.valueplan.rs).folded()unnest_display_name.folded()folded()matchesexpr_build::fold_identexactly — the established rule for ordinary identifiers.Commit structure
Task 1 is a deliberate pure refactor (
34b61be):QualifiedNamethreaded through with every consumer taking.value— byte-for-byte the oldString— and the suite proven identical at 572/12 with the target test still xfailing. Behaviour changes only ina5b0059. A regression is therefore attributable to one commit rather than tangled with the refactor.Two scope corrections found during implementation
1. The planned
unnest(T.s) FROM ttest was impossible — resolving a qualifier whose case differs from the relation needs relation-alias folding, the forbidden path, and both engines fail it identically. Dropped.2. The final review caught that
a5b0059's message then over-claimed — it said the unnest fold was dropped, but only the test was. The fold shipped with zero coverage: mutating it back to.valuepassed the whole suite. Fixed inebafe1d, which covers it using a relation registered asT(DataFusion renders the registered, folded qualifier into unnest output names, soTyieldst.s.x).Pre-existing codegen bugs exposed (not caused)
Both pinned
xfail_on_codegen(strict=True), codegen's Python untouched by this branch:"S".xwhere the oracle rejects it (over-folds a quoted qualifier) — verified onorigin/masterbefore this branch.T.s.x) instead of the oracle'st.s.x.Both want their own ticket rather than an inline fix, per the file-ownership rule.
Verification
folded()to always fold breaks the quoted test; never folding breaks the unquoted test.check_both_raisehardened withmatch="S"— it previously accepted any error from either engine and could have passed for an unrelated reason.sql_transform/_codegen/plan.pyortests/test_codegen_coverage.py.Out of scope
Relation qualifiers still diverge from the oracle, which registers relations under a folded name (
"__THIS__".agemisses there, hits on native). Measured, unreachable through the public API, fails loudly. Different branch, second core change — deliberately not folded in, and tracked separately.🤖 Generated with Claude Code