feat(codegen): struct/list containers — TASK-29 Phase B (Tasks 1–2 of 4) - #4
Conversation
34616f6 to
9f52e9e
Compare
9f52e9e to
b03887e
Compare
ahrzb
changed the title
b03887e to
7410b90
Compare
|
Claude finished @claude-agent-ahrzb[bot]'s task in 13m 10s —— View job Review complete
Summary: No Critical issues. One Important finding: Full review posted as a top-level PR comment plus 2 inline comments on Verdict: Ready to merge with fixes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # struct(...) parses as exp.Struct and array(...) as exp.Array (handled | ||
| # above); only make_array reaches the Anonymous arm. | ||
| if name == "make_array": | ||
| return ListExpr([_convert_expr(a) for a in e.expressions]) |
There was a problem hiding this comment.
Important — test gap: make_array(...) (this branch), bare positional struct(a, b), and the aliased STRUCT(x AS name, ...) form (both handled by _convert_struct below, lines 422-426) have no test coverage anywhere in the diff. I checked tests/test_codegen_coverage.py's _COMMITTED list and tests/test_diff_types.py: only named_struct(...) and array(x, y)/bracket-list [x, y] are exercised for value correctness. A latent bug in the make_array dispatch or in the PropertyEQ/Alias branches of _convert_struct would go undetected.
Recommend adding a _COMMITTED coverage-guard entry plus a differential.check() value-assertion test for each of these three shapes before merging, matching the plan's own "no test = not done" bar.
| if isinstance(e, exp.Struct): | ||
| # sqlglot parses named_struct('k', v, ...) and struct(a, b, ...) authored | ||
| # via STRUCT(...) into exp.Struct with exp.PropertyEQ / bare exprs. |
There was a problem hiding this comment.
Minor — stale comment: this says named_struct(...) parses into exp.Struct, but the code below (lines 394-395) still separately dispatches named_struct from the exp.Anonymous arm via _named_struct, and test_build_plan_constructs_struct_and_list (tests/test_codegen_coverage.py around line 245-247) confirms that's the path actually taken — it expects StructExpr([("k", ...)]) with the literal key name, not the c0/c1 positional-fallback shape _convert_struct would produce. Worth correcting so a future maintainer doesn't mistake the named_struct branch in the Anonymous arm for dead code (the way struct/array were correctly removed as dead code in this PR's last commit).
Review: Tasks 1–2 of Phase B (struct/list codegen)Reviewed Strengths
Important (should fix before merge)
Minor (nice to have)
Assessment: With fixes. Architecture and the reviewed logic are sound and correctly scoped; add tests for the three untested construction surfaces (and optionally the two stale-comment fixes) before merging. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntainer output (TASK-29B-1) Move struct and list column projection from deferred (UnsupportedInCodegen) to committed surface. The output model + marshalling infrastructure already handles struct/list Pydantic models. This unlocks struct/list passthrough for codegen, matching native engine parity. DataFusion oracle validates. Changes: - sql_transform/_codegen/engine.py: Remove the guard block rejecting struct/list containers - tests/test_codegen_coverage.py: Move struct/list passthroughs from _DEFERRED to _COMMITTED - sql_transform/_codegen/engine_test.py: Update test to verify struct columns now pass through Test results: 495 passed (+3), 11 skipped (-3) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…ocstring (TASK-29B-1 review) - Remove unused UnsupportedInCodegen import from engine_test.py (F401) - Fix stray blank lines in engine_test.py via ruff format - Update _to_native docstring: struct columns now reach projected output via passthrough, not only via JOIN keys; removed false claim about UnsupportedInCodegen rejection Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… TASK-29 Phase B
Adds StructExpr/ListExpr IR: named_struct(...), struct(...)/STRUCT(...), and
array(...)/make_array(...)/[...] now build a Python dict/list at runtime
instead of raising UnsupportedInCodegen. _DEFERRED_FUNCS shrinks to
("unnest",).
Also tightens the Func emit path in engine.py: an unrecognized function name
(notably a transformer call, __tfm_N__) now raises UnsupportedInCodegen
instead of a generic ValueError -- this path was previously unreachable
because named_struct short-circuited before descending into its arguments;
completing named_struct exposed it. Updated plan_test.py's stale
"named_struct defers" assertion accordingly and added a focused unit test for
the new construction IR.
… defer to __tfm_ (TASK-29B-2 review) - infer_type's ListExpr arm now unifies element types via _common_base (same helper COALESCE uses), so make_array(int, float) types as list[float] and pydantic widens the int element at model_validate (measured: no emission-side change needed). - _emit_expr's Func branch narrows the UnsupportedInCodegen defer to __tfm_ transformer placeholders only; every other unknown function name (e.g. foobar(x), invalid SQL) keeps the original hard ValueError instead of being silently skipped by the differential harness.
…B review) struct(...) parses as exp.Struct and array(...) as exp.Array, both handled by earlier _convert_expr arms; only make_array reaches the Anonymous arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7410b90 to
76d14f4
Compare
…comments (PR#4 review) Auto-review flagged three committed codegen construction shapes with no value test: positional struct(a,b) -> c0/c1, named struct(a AS x,...) (exp.PropertyEQ), and make_array(...). All three match the DataFusion oracle on codegen but native has no dispatch for them, so they xfail_on_native -- same parity-gap pattern as the mixed-numeric widening test. Native gaps want their own tickets. Also fixed two stale comments: plan.py claimed named_struct parses to exp.Struct (it's exp.Anonymous), and test_transformer_case claimed codegen defers on named_struct (it defers on the __tfm_0__ transformer call; named_struct is supported now). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in Important (test gap): Added value-asserting differential tests for all three untested construction shapes — positional Minor (stale comments): Both fixed. Re the Suite: 515 passed, 9 skipped, 4 xfailed. Not adding empty/nested/NULL-element edge tests unless wanted — the recursive path is exercised by the existing cases. |
(TASK-45 stretch 4) Counting-allocator tests now pin zero heap allocation per warm run for the interpreter AND cranelift, over the probe/arith fixture and a string-heavy program (case, trim, substr, concat, itos/ftos, parse, compare). What had to change: substr and trim become pure sub-span arithmetic (byte ranges into the input span — no copy at all), case mapping streams char-at-a-time into the arena (Arena::case_map), number->text formats straight into the arena (Arena::push_fmt) with DuckF64 rendering to a stack buffer, string compare/parse read the arena immutably, and h_probe emits values without the per-call Vec + ScalarVal clones. All shared between backends; 500-seed differential and the full 612-test pytest suite stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(TASK-45 stretch 4) Counting-allocator tests now pin zero heap allocation per warm run for the interpreter AND cranelift, over the probe/arith fixture and a string-heavy program (case, trim, substr, concat, itos/ftos, parse, compare). What had to change: substr and trim become pure sub-span arithmetic (byte ranges into the input span — no copy at all), case mapping streams char-at-a-time into the arena (Arena::case_map), number->text formats straight into the arena (Arena::push_fmt) with DuckF64 rendering to a stack buffer, string compare/parse read the arena immutably, and h_probe emits values without the per-call Vec + ScalarVal clones. All shared between backends; 500-seed differential and the full 612-test pytest suite stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Teaches the codegen engine to handle SQL struct/list containers as output and as constructed values — retiring 5 of the container-related differential skips (14 → 9). This is the reviewed, shippable first half of Phase B; field access and comparison (Tasks 3–4) are not in this PR.
Plan:
docs/superpowers/plans/2026-07-22-codegen-deferred-phase-b-containers.mdWhat's in this PR
88e488ca29b194CodegenFn.__init__so a struct/list column flows through codegen (output model = pydantic submodel /list[...];_to_nativeunwraps to dict/list)32e2df7_to_nativedocstring)62b890fStructExpr/ListExprthreaded through_convert_expr → infer_type → _validate_expr → _emit_expr, coveringnamed_struct(...),struct(...),array(...)/make_array(...)0558fc9__tfm_prefix so genuinely unknown funcs hard-errorb03887estruct/arraybranches in theexp.Anonymousarm (verified viasqlglot.parse_onethat those route throughexp.Struct/exp.Arrayearlier; onlymake_arrayreaches Anonymous)Files touched
sql_transform/_codegen/plan.py—StructExpr/ListExprIR nodes + convert/infer/validate arms; trimmed_DEFERRED_FUNCSsql_transform/_codegen/engine.py— remove container-output raise;_emit_exprconstruction armstests/test_codegen_coverage.py— container shapes moved_DEFERRED → _COMMITTEDdocs/superpowers/plans/2026-07-22-codegen-deferred-phase-b-containers.md— the planScope limited to
sql_transform/_codegen/andtests/— no native-engine changes. DataFusion is the oracle; struct values are Python dicts with significant key order; scalar equality inside containers is type-tagged.Status — Phase B is half done
named_struct/struct/array)s.x, nested)Tasks 3–4 close the remaining container skips; the final 5 skips are UNNEST, deliberately left for Phase C.
Review & tests
requesting-code-reviewon Tasks 1–2 → ready to merge, no Critical/Important issues; the one Minor finding (dead branches) is fixed inb03887e.uv run pytest→ 512 passed, 9 skipped, 1 xfailed (skip count down from 14).testCI check green. (Thereviewcheck is theanthropics/claude-code-actionauto-reviewer failing on its own infra, unrelated to this diff.)🤖 Generated with Claude Code