Skip to content

Commit 141b939

Browse files
ahrzbclaude
ahrzb
and
claude
committed
docs: TASK-29 Phase A plan — unary minus (non-literal) + || operator
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6a36b2f commit 141b939

1 file changed

Lines changed: 198 additions & 0 deletions

File tree

‎docs/superpowers/plans/2026-07-20-codegen-deferred-phase-a-operators.md‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
# Codegen deferred surface — Phase A (scalar operators) Implementation Plan — TASK-29
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Make the codegen engine evaluate unary-minus-on-a-non-literal and the `||` operator, retiring their 2 differential-suite skips (16 → 14).
6+
7+
**Architecture:** Both are scalar and reuse codegen's existing `BinaryOp` infra. Unary minus on a non-literal lowers to `0 - x` (mirrors native `expr_build.rs`), reusing `rt.sub`. `||` becomes a `BinaryOp("dpipe", …)` emitting to a new NULL-propagating concat runtime helper (mirrors native `expr.rs` `concat_op`). No new IR relational surface.
8+
9+
**Tech Stack:** Python 3, sqlglot, pyarrow, pydantic, pytest, DataFusion (oracle), maturin (native build for the differential harness's native backend).
10+
11+
## Global Constraints
12+
13+
- Native is the reference; DataFusion is the oracle. Where an engine disagrees with DataFusion, the engine is wrong.
14+
- Test bar (no test = not done): each shape flips from skipped to green on the codegen backend in the differential suite AND moves into `_COMMITTED` in `tests/test_codegen_coverage.py`.
15+
- v0, no backward-compat; direct changes, no shims.
16+
- Scope stays in `sql_transform/_codegen/` and `tests/`. No native-engine or fit/state/rewrite changes.
17+
- The differential harness's native backend imports the built `_interpreter`; ensure it's built with `uv run maturin develop` before running the suite. Run tests with `uv run pytest`.
18+
19+
## Pre-req: native build + baseline
20+
21+
Before Task 1, ensure the native extension is built so the differential harness's `native` backend works, and record the baseline skip count:
22+
23+
```
24+
uv run maturin develop
25+
uv run pytest -q # record the "N skipped" number — Phase A should drop it by the unary-minus + || codegen skips
26+
```
27+
28+
## File Structure
29+
30+
- **Modify** `sql_transform/_codegen/plan.py` — `_convert_expr` (`exp.Neg` non-literal ~line 317; `exp.DPipe` ~line 326); `infer_type` `BinaryOp` arm (~line 700) for the `dpipe` op.
31+
- **Modify** `sql_transform/_codegen/engine.py` — `_OPS` map (~line 29) add `dpipe`.
32+
- **Modify** `sql_transform/_codegen/runtime.py` — add `dpipe` helper (near `concat`, ~line 392).
33+
- **Modify** `tests/test_codegen_coverage.py` — `_COMMITTED` list (~line 16) + the stale NB comment about unary-minus/`||`.
34+
35+
The existing differential tests `tests/test_diff_rust_bugs.py::test_unary_minus` and `::test_string_concat_operator` already cover both shapes (they skip on codegen today, pass on native); they need no change — implementing the shapes flips them green on codegen.
36+
37+
---
38+
39+
### Task 1: unary minus on a non-literal
40+
41+
**Files:**
42+
- Modify: `sql_transform/_codegen/plan.py` (`_convert_expr` `exp.Neg` branch)
43+
- Modify: `tests/test_codegen_coverage.py` (`_COMMITTED` + NB comment)
44+
45+
**Interfaces:**
46+
- Consumes: existing `BinaryOp`, `Literal`, `_convert_expr`, `infer_type` `sub` rule, `engine._OPS["sub"]`.
47+
- Produces: codegen accepts `-x` for a non-literal `x` (as `BinaryOp("sub", Literal(0), x)`).
48+
49+
- [ ] **Step 1: Commit the shape to the coverage guard (RED)**
50+
51+
In `tests/test_codegen_coverage.py`, add to the `_COMMITTED` list (e.g. after the `NULLIF`/`CAST` block) an entry for unary minus on a non-literal:
52+
53+
```python
54+
("SELECT -a AS x FROM t", {"t": rows({"a": "int"}, [{"a": 5}])}),
55+
```
56+
57+
And edit the existing NB comment (which currently says unary minus AND `||` are deliberately absent) so it no longer claims unary minus is deferred — leave only `||` mentioned for now:
58+
59+
```python
60+
# NB: the `||` operator is deliberately absent here until Phase A Task 2 lands
61+
# it -- see the codegen deferred-surface spec.
62+
```
63+
64+
- [ ] **Step 2: Run the coverage guard to verify RED**
65+
66+
Run: `uv run pytest tests/test_codegen_coverage.py::test_committed_surface_is_never_deferred -q`
67+
Expected: FAIL — the new `-a` entry raises `UnsupportedInCodegen` ("unary minus on a non-literal is not supported in codegen yet"), which `test_committed_surface_is_never_deferred` reports as a failure.
68+
69+
- [ ] **Step 3: Implement — lower unary minus to `0 - x`**
70+
71+
In `sql_transform/_codegen/plan.py` `_convert_expr`, replace the `exp.Neg` branch's `raise UnsupportedInCodegen(...)` for the non-literal case with a lowering to `BinaryOp("sub", Literal(0), inner)`:
72+
73+
```python
74+
if isinstance(e, exp.Neg):
75+
inner = _convert_expr(e.this)
76+
if isinstance(inner, Literal) and type(inner.value) in (int, float):
77+
return Literal(-inner.value)
78+
# Unary minus on a non-literal: lower to 0 - x, mirroring native
79+
# (expr_build.rs), reusing Sub's int->int / float->float promotion and
80+
# NULL propagation. infer_type/emit already handle "sub".
81+
return BinaryOp("sub", Literal(0), inner)
82+
```
83+
84+
(Delete the two-line `raise UnsupportedInCodegen("unary minus on a non-literal ...")` that was there.)
85+
86+
- [ ] **Step 4: Run coverage guard + the differential test to verify GREEN**
87+
88+
Run: `uv run pytest tests/test_codegen_coverage.py -q`
89+
Expected: PASS — `-a` is now committed surface.
90+
91+
Run: `uv run pytest tests/test_diff_rust_bugs.py::test_unary_minus -q`
92+
Expected: PASS on both backends — `-a` (int and float) now evaluates on codegen and matches the DataFusion oracle (native already passed). No skip.
93+
94+
- [ ] **Step 5: Commit**
95+
96+
```bash
97+
git add sql_transform/_codegen/plan.py tests/test_codegen_coverage.py
98+
git commit -m "feat(codegen): unary minus on a non-literal (lower to 0 - x) — TASK-29 Phase A"
99+
```
100+
101+
---
102+
103+
### Task 2: the `||` operator
104+
105+
**Files:**
106+
- Modify: `sql_transform/_codegen/runtime.py` (add `dpipe`)
107+
- Modify: `sql_transform/_codegen/plan.py` (`_convert_expr` `exp.DPipe`; `infer_type` `BinaryOp`)
108+
- Modify: `sql_transform/_codegen/engine.py` (`_OPS`)
109+
- Modify: `tests/test_codegen_coverage.py` (`_COMMITTED` + remove NB comment)
110+
111+
**Interfaces:**
112+
- Consumes: `runtime.display`, existing `BinaryOp` IR, `engine._emit_expr` `BinaryOp` path, `plan.infer_type` `BinaryOp` arm.
113+
- Produces: codegen accepts `a || b` as `BinaryOp("dpipe", a, b)`, typed `STR`, evaluated NULL-propagating.
114+
115+
- [ ] **Step 1: Commit the shape to the coverage guard (RED)**
116+
117+
In `tests/test_codegen_coverage.py`, add to `_COMMITTED`:
118+
119+
```python
120+
("SELECT a || '!' AS x FROM t", {"t": rows({"a": "str"}, [{"a": "hi"}])}),
121+
```
122+
123+
And DELETE the NB comment about `||` being deferred (both operators are committed after this task).
124+
125+
- [ ] **Step 2: Run the coverage guard to verify RED**
126+
127+
Run: `uv run pytest tests/test_codegen_coverage.py::test_committed_surface_is_never_deferred -q`
128+
Expected: FAIL — the `a || '!'` entry raises `UnsupportedInCodegen` ("the || operator is not supported in codegen yet").
129+
130+
- [ ] **Step 3: Add the NULL-propagating concat runtime helper**
131+
132+
In `sql_transform/_codegen/runtime.py`, add near `concat` (~line 392):
133+
134+
```python
135+
def dpipe(l: Any, r: Any) -> Any: # noqa: E741
136+
"""The `||` operator: NULL-propagating string concat -- any NULL operand
137+
yields NULL (unlike concat(), which skips NULLs). Mirrors expr.rs concat_op:
138+
non-NULL operands are rendered via display() and joined."""
139+
if l is None or r is None:
140+
return None
141+
return display(l) + display(r)
142+
```
143+
144+
- [ ] **Step 4: Convert `exp.DPipe` and type it**
145+
146+
In `sql_transform/_codegen/plan.py` `_convert_expr`, replace the `exp.DPipe` branch's `raise UnsupportedInCodegen(...)` with a lowering to a `dpipe` `BinaryOp` (binary; `a||b||c` nests as `DPipe(DPipe(a,b), c)`, so `.this`/`.expression` recursion handles it):
147+
148+
```python
149+
if isinstance(e, exp.DPipe):
150+
# The `||` operator: NULL-propagating string concat (mirrors native
151+
# concat_op). Binary -- a||b||c nests as DPipe(DPipe(a,b), c).
152+
return BinaryOp("dpipe", _convert_expr(e.this), _convert_expr(e.expression))
153+
```
154+
155+
In `sql_transform/_codegen/plan.py` `infer_type`, in the `BinaryOp` arm, add a `dpipe` case that types as `STR` BEFORE the container check and the `BOOL` fallthrough (place it right after the arithmetic-ops `if` block):
156+
157+
```python
158+
if e.op == "dpipe":
159+
return FieldType(STR, nullable)
160+
```
161+
162+
- [ ] **Step 5: Emit `dpipe` to the runtime helper**
163+
164+
In `sql_transform/_codegen/engine.py`, add to the `_OPS` map:
165+
166+
```python
167+
"dpipe": "rt.dpipe",
168+
```
169+
170+
- [ ] **Step 6: Run coverage guard + the differential test to verify GREEN**
171+
172+
Run: `uv run pytest tests/test_codegen_coverage.py -q`
173+
Expected: PASS — `a || '!'` is committed surface.
174+
175+
Run: `uv run pytest tests/test_diff_rust_bugs.py::test_string_concat_operator -q`
176+
Expected: PASS on both backends — `a || '!'` (string), `a || NULL` (→ NULL), and `a || 5` (int coerced via display → "hi5") all evaluate on codegen and match the DataFusion oracle. No skip.
177+
178+
- [ ] **Step 7: Run the full suite and confirm the skip delta**
179+
180+
Run: `uv run pytest -q`
181+
Expected: PASS — no regressions; the "N skipped" count is lower than the pre-req baseline by the unary-minus + `||` codegen skips (Phase A retires 2 shapes). Note the before/after skip numbers for the merge report.
182+
183+
- [ ] **Step 8: Commit**
184+
185+
```bash
186+
git add sql_transform/_codegen/runtime.py sql_transform/_codegen/plan.py \
187+
sql_transform/_codegen/engine.py tests/test_codegen_coverage.py
188+
git commit -m "feat(codegen): || operator (NULL-propagating concat) — TASK-29 Phase A"
189+
```
190+
191+
---
192+
193+
## Self-Review
194+
195+
- **Spec coverage (Phase A):** unary-minus-on-non-literal (Task 1) ✓; `||` (Task 2) ✓. Both mirror native and are proven by the existing differential tests flipping green on codegen + `_COMMITTED` entries ✓. No native/fit-path changes ✓.
196+
- **Placeholder scan:** none — every step has concrete code and exact run/expected lines.
197+
- **Type consistency:** the `dpipe` op string is identical across `_convert_expr` (Task 2 Step 4), `infer_type` (Step 4), and `engine._OPS` (Step 5); `rt.dpipe` name matches the runtime helper (Step 3).
198+
- **Note:** unary minus needs no `infer_type`/emit change because it reuses the existing `sub` `BinaryOp` path; only the conversion changes.

0 commit comments

Comments
 (0)