gregfelice commented on PR #2435:
URL: https://github.com/apache/age/pull/2435#issuecomment-4763292009
Reviewed the full diff and the changed files in context, with two deep
passes on the executor/planner mechanics. I independently verified the two
subtle correctness traps this kind of desugaring is prone to — the
COALESCE-NULL ambiguity and PARAM_EXEC isolation — and both are genuinely
handled. Solid work; no blockers or majors. (Source-level review; I could not
run `make` here — the 38/38 installcheck is the author's.)
### Verified correct
- **The empty-list vs. fold-to-null ambiguity is correctly resolved** — the
trap I most expected here. `age_reduce_transfn` never returns SQL NULL: a null
fold result is normalized to an agtype `'null'` varlena
(`agtype.c:11730-11733`), and the accumulator is seeded the same way. So the
aggregate returns SQL NULL *only* over zero rows. The desugaring `CASE WHEN
list IS NULL THEN NULL ELSE COALESCE(<agg>, init) END` therefore reads
correctly: empty list -> SQL NULL -> `init`; a legitimate fold-to-null ->
agtype `'null'` (not SQL NULL) -> passes through COALESCE. The `[1, null, 3] ->
null` test (`age_reduce.sql:186`) exercises exactly this. The classic bug is
not present.
- **By-value Datum safety holds.** The body is normalized to agtype at
transform time, and the transfn independently re-checks `exprType(body_node) ==
AGTYPEOID` at runtime (`agtype.c:11680`) because `age_reduce` is SQL-callable,
so `datumCopy(result, false, -1)` can never misread a by-value bool/int as a
varlena. Good defense in depth.
- **Accumulator survives transitions** — `datumCopy`'d into `aggcontext`,
not the per-tuple context; `PG_ARGISNULL(0)` is a reliable first-element signal
precisely because the state is never SQL NULL.
- **PARAM_EXEC 0/1 cannot collide with planner params.** The body is
evaluated in a private standalone ExprContext whose `ecxt_param_exec_vals` is
the transfn's own 2-element array; the outer planner never sees ids 0/1, and no
node type that would need a parent (Aggref/SubLink/WindowFunc) can reach the
body — all are rejected before serialization.
- **Nested-reduce / outer-var / param rejection is enforced at transform
time** (a nested reduce lowers to a SubLink, which `reduce_body_check_walker`
rejects; the `varlevelsup == 0` guard prevents an outer RTE sharing a varno
from being rewritten into the accumulator param). The three rejection tests
confirm it.
- **Init-evaluated-once is correct.** Wrapping init in `CASE WHEN
reduce_ordinality = 1 THEN <init> ELSE NULL::agtype END` is sound because the
aggregate-level ORDER BY delivers ordinality 1 first and the transfn only reads
init when the state is NULL; `PARALLEL UNSAFE` + no combinefn prevents any
partial-aggregate reordering.
- **Serialization plumbing is complete and symmetric** — `cypher_reduce` is
wired into the tag enum, `node_names[]`/`node_methods[]`, all five fields in
copy/out/read with matching headers, and the raw-expr walker in
`cypher_analyze.c` descends all three expr fields. No field missed.
- **Backward compat preserved** — `reduce` is added to the `safe_keywords`
grammar production (like `any`/`none`/`order`), and `{reduce: 1}` still works
as a map key.
### Test coverage gaps (cheap to close, no bug implied)
The mechanisms below are correct by inspection, but the suite doesn't lock
in the exact semantics that make them correct:
1. **A fold that legitimately produces null mid-way and then recovers**
(e.g. a `coalesce`/CASE body that climbs back out of null). Only "null
propagates to a null result" is tested; the recovery path *through* the
agtype-`'null'` state is the load-bearing one and isn't directly exercised.
2. **Empty list with a NULL init** (`reduce(s = null, x IN [] | ...)`) — the
`COALESCE(NULL, null)` path is unverified.
3. **A type error / runtime failure in the body** (e.g. a non-coercible or
arithmetic-invalid body) — the `coerce_to_common_type(..., "reduce")` and
runtime error paths are unexercised.
(Large-list / interrupt-path stress and deeper nesting are nice-to-haves;
the SubLink check makes nesting depth irrelevant.)
Adding #1–#3 would be a few lines each and would pin down precisely the
behavior this implementation gets right. Nice piece of work overall — the
agtype-`'null'` state trick and the standalone-ExprContext param isolation are
the right calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
