feat(codegen): struct field access + container equality — TASK-29 Phase B (Tasks 3–4 of 4) - #9
Merged
Merged
Conversation
FieldAccess IR node threaded through convert/validate/infer/emit + rt.getfield. _convert_expr layers FieldAccess over multi-part columns (catalog/db/table qualifiers + leaf); the 2-part s.x stays a Column and is rewritten to FieldAccess in _validate_expr once schemas show the qualifier isn't a relation alias (mirrors native plan.rs). _validate_expr now returns the (possibly rewritten) node so callers reassign. Covers s.x, nested s.a.b, qualified t.s.x against the DataFusion oracle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…29 Phase B (Task 4) rt._veq: type-tagged deep equality (structs by key order+values, lists elementwise, scalars via val_eq so Int(1) != Float(1.0)); rt.eq/neq route containers through it. infer_type's BinaryOp arm now checks containers FIRST: eq/neq -> BOOL, every other op (arithmetic, dpipe, ordering) defers. That ordering also closes the Phase-A carryover where 'struct || x' mistyped as STR instead of deferring. Struct/list equality matches the DataFusion oracle on both backends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Tasks 3-4 review) Review of Tasks 3-4 flagged an unquoted uppercase struct-column qualifier not being folded: S.x built a lookup for column 'S' while DataFusion folds it to 's'. Fold the qualifier when the validate rewrite reinterprets it as a column (safe: the relation lookup already matched it raw and missed). Locked with an xfail_on_native diff test -- native has the same (worse) gap, wants a ticket. Also: comment the deliberate drop of the plan's 4-part catalog raise (lets t.s.a.b resolve via layered FieldAccess), and add list-inequality coverage. Left unfixed (noted for tickets): struct=scalar returns False where DataFusion type-errors; field access on a NULL struct (DataFusion returns 0, not NULL) -- both out of Phase B's committed surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
/review |
|
I'll analyze this and get back to you. |
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.

Summary
Completes Phase B of the codegen deferred surface: struct field access and struct/list equality. With Tasks 1–2 (merged in #4) this retires every container differential skip except UNNEST — the 5 remaining skips are all UNNEST, deliberately left for Phase C.
Plan:
docs/superpowers/plans/2026-07-22-codegen-deferred-phase-b-containers.md(Tasks 3–4).What's in this PR
bee2b39FieldAccessIR node threaded through_convert_expr/infer_type/_validate_expr/_emit_expr+runtime.getfield._convert_exprlayersFieldAccessover multi-part columns (catalog/db/table qualifiers + leaf); the 2-parts.xstays aColumnand is rewritten toFieldAccessin_validate_expronce schemas show the qualifier isn't a relation alias (mirrors nativeplan.rs)._validate_exprnow returns the (possibly rewritten) node; every caller reassigns. Coverss.x, nesteds.a.b, qualifiedt.s.x.7a502a9runtime._veq(type-tagged deep equality: structs by key order+values, lists elementwise, scalars viaval_eqsoInt(1) != Float(1.0));rt.eq/neqroute containers through it.infer_type'sBinaryOparm checks containers first —eq/neq→ BOOL, every other op defers — which also closes the Phase-A carryover wherestruct || xmistyped as STR.795e3e9S.x→s.x, matching DataFusion); comment the deliberate drop of the 4-partcatalograise (letst.s.a.bresolve); add list-inequality coverage.Testing
uv run pytest→ 530 passed, 5 skipped (all UNNEST/Phase C), 5 xfailed. All field-access and equality shapes are value-asserted against the DataFusion oracle on both backends (native + codegen).s.x, nesteds.a.b, qualifiedt.s.x(all pass on codegen).xfail_on_native(codegen matches oracle, native has the gap): mixed-numeric list widening + the new uppercase-qualifier fold.Scope limited to
sql_transform/_codegen/andtests/— no native-engine changes.Known limitations (out of Phase B's committed surface — flagged for tickets)
make_arrayconstruction or fold struct-column qualifiers. Documented viaxfail_on_native.struct = scalarreturnsFalseon codegen where DataFusion type-errors at planning.0(not NULL); neither engine matches. Worth its own investigation.Review
Ran
requesting-code-reviewon Tasks 3–4 (including independent vetting of the inherited Task 3 code) → ready to merge, no Critical/Important issues. The795e3e9fixes address the Minor findings; the remaining two are the noted out-of-scope limitations above.🤖 Generated with Claude Code