You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@age.apache.org by "jrgemignani (via GitHub)" <gi...@apache.org> on 2026/06/27 00:18:11 UTC

[PR] Support outer references in reduce() fold bodies [age]

jrgemignani opened a new pull request, #2448:
URL: https://github.com/apache/age/pull/2448

   Allow a reduce(acc = init, var IN list | body) fold body to reference loop-invariant values from the enclosing query -- outer-query variables and cypher() parameters -- in addition to the accumulator and element. These were previously rejected with ERRCODE_FEATURE_NOT_SUPPORTED.
   
   How it works
   ------------
   The fold body is still compiled to a standalone expression evaluated by age_reduce_transfn, so an outer reference (which cannot be evaluated there) is captured at transform time and supplied as a value:
   
     - After the accumulator and element are rewritten to PARAM_EXEC params 0 and 1, transform_cypher_reduce() walks the body and replaces each maximal agtype-typed, loop-invariant subtree -- one that references an outer Var or a cypher() $parameter but not the accumulator/element -- with a new PARAM_EXEC param 2, 3, ... in body order.
     - The captured expressions are passed to the aggregate as a trailing agtype[] argument; age_reduce(agtype, text, agtype, agtype[]) and its transition function gain this argument.
     - age_reduce_transfn sizes its param array to 2 + the number of captures and binds the captured values to params 2.. on every row. Because the captures are evaluated in the outer query context as ordinary aggregate arguments, a correlated capture is re-evaluated per group, so an outer value that varies per row (for example under UNWIND) is folded with the correct value.
   
   This keeps the no-core-patch design: the body is still a serialized standalone expression, and the only new machinery is the captured-value plumbing.
   
   Still rejected
   --------------
   Subqueries in the body (including a nested reduce()) and aggregate functions remain unsupported and raise a clean ERRCODE_FEATURE_NOT_SUPPORTED error: a subquery cannot be planned as a plain aggregate argument, and an aggregate in a per-element fold is undefined per the openCypher specification.
   
   Tests
   -----
   age_reduce gains an "Outer references in the fold body" section covering a plain outer variable, an outer variable used as a multiplier, two distinct outer variables, a property of an outer graph variable, the same outer variable referenced more than once, a property of an outer map, a subexpression that mixes an outer reference with the element (only the loop-invariant part is captured), an outer reference inside a CASE branch of the body, a NULL outer value propagating through the fold, multiple captures mixing a NULL and a non-NULL outer value, an outer variable that changes per row (captured per group), and a cypher() parameter supplied via a prepared statement. The previously-rejected outer-variable case is moved out of the not-supported section, which now covers a nested reduce() (any subquery in the body is unsupported) and an aggregate in the body.
   
   The same change also broadens the base reduce() coverage with value-type folds (a float accumulator, negative numbers, a map accumulator passed through unchanged, and list elements indexed in the body), function calls in the fold body (a scalar function over the element and the list itself produced by a function), reduce() composed with surrounding expressions (consumed by another function and used in a comparison), and syntax-error checks for each required piece of the form -- the "= init", ", var IN list", and "| body" clauses, plus a rejected qualified iterator variable. 42/42 installcheck pass.
   
   Co-authored-by: Copilot <co...@github.com>
   
   modified:   age--1.7.0--y.y.y.sql
   new file:   regress/age_load/data/junk.csv
   modified:   regress/expected/age_reduce.out
   modified:   regress/sql/age_reduce.sql
   modified:   sql/age_aggregate.sql
   modified:   src/backend/parser/cypher_clause.c
   modified:   src/backend/utils/adt/agtype.c


-- 
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support outer references in reduce() fold bodies [age]

Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.

jrgemignani commented on code in PR #2448:
URL: https://github.com/apache/age/pull/2448#discussion_r3484720135


##########
regress/age_load/data/junk.csv:
##########
@@ -0,0 +1,54 @@
+id,name,iso3,iso2,numeric_code,phone_code,capital,currency,currency_symbol,tld,native,region,subregion,latitude,longitude,emoji,emojiU
+2,Aland Islands,ALA,AX,248,+358-18,Mariehamn,EUR,€,.ax,Åland,Europe,Northern Europe,60.116667,19.9,🇦🇽,U+1F1E6 U+1F1FD
+3,Albania,ALB,AL,8,355,Tirana,ALL,Lek,.al,Shqipëria,Europe,Southern Europe,41.0,20.0,🇦🇱,U+1F1E6 U+1F1F1
+6,Andorra,AND,AD,20,376,Andorra la Vella,EUR,€,.ad,Andorra,Europe,Southern Europe,42.5,1.5,🇦🇩,U+1F1E6 U+1F1E9
+15,Austria,AUT,AT,40,43,Vienna,EUR,€,.at,Österreich,Europe,Western Europe,47.33333333,13.33333333,🇦🇹,U+1F1E6 U+1F1F9

Review Comment:
   Added by accident, removed.



-- 
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support outer references in reduce() fold bodies [age]

Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.

jrgemignani commented on PR #2448:
URL: https://github.com/apache/age/pull/2448#issuecomment-4861653410

   > @jrgemignani **GPT 5.5 xhigh review**:
   > 
   > I found one semantic issue with the capture rewrite.
   > 
   > `reduce_capture_mutator()` captures the largest agtype-typed outer-reference subtree and passes it through the `extras` aggregate argument. That means the captured expression is evaluated unconditionally as an aggregate argument, before the fold body runs. This changes short-circuit behavior for `CASE`, `AND`, `OR`, etc.
   > 
   > Repro:
   > 
   > ```sql
   > WITH 0 AS z
   > RETURN reduce(s = 0, x IN [1] |
   >   CASE WHEN false THEN s + 1 / z ELSE s END)
   > ```
   > 
   > This currently raises division by zero, even though the branch should not be evaluated. The same expression without an outer capture returns 0.
   > 
   > I think the capture needs to preserve evaluation order, likely by capturing only outer leaf values and leaving operators/function calls inside the serialized fold expression.
   > 
   > Also, `git diff --check apache/master...pr-2448` reports trailing whitespace in the newly added regress/expected/age_reduce.out lines.
   
   Resolved.


-- 
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support outer references in reduce() fold bodies [age]

Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.

MuhammadTahaNaveed commented on PR #2448:
URL: https://github.com/apache/age/pull/2448#issuecomment-4856755480

@jrgemignani
**GPT 5.5 xhigh review**:

I found one semantic issue with the capture rewrite.

`reduce_capture_mutator()` captures the largest agtype-typed outer-reference subtree and
passes it through the `extras` aggregate argument. That means the captured expression is
evaluated unconditionally as an aggregate argument, before the fold body runs. This changes
short-circuit behavior for `CASE`, `AND`, `OR`, etc.

Repro:

```sql
WITH 0 AS z
RETURN reduce(s = 0, x IN [1] |
CASE WHEN false THEN s + 1 / z ELSE s END)
```
This currently raises division by zero, even though the branch should not be evaluated. The
same expression without an outer capture returns 0.

I think the capture needs to preserve evaluation order, likely by capturing only outer leaf
values and leaving operators/function calls inside the serialized fold expression.

Also, git diff --check apache/master...pr-2448 reports trailing whitespace in the newly
added regress/expected/age_reduce.out lines.

--
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support outer references in reduce() fold bodies [age]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #2448:
URL: https://github.com/apache/age/pull/2448#discussion_r3484716493


##########
regress/age_load/data/junk.csv:
##########
@@ -0,0 +1,54 @@
+id,name,iso3,iso2,numeric_code,phone_code,capital,currency,currency_symbol,tld,native,region,subregion,latitude,longitude,emoji,emojiU
+2,Aland Islands,ALA,AX,248,+358-18,Mariehamn,EUR,€,.ax,Åland,Europe,Northern Europe,60.116667,19.9,🇦🇽,U+1F1E6 U+1F1FD
+3,Albania,ALB,AL,8,355,Tirana,ALL,Lek,.al,Shqipëria,Europe,Southern Europe,41.0,20.0,🇦🇱,U+1F1E6 U+1F1F1
+6,Andorra,AND,AD,20,376,Andorra la Vella,EUR,€,.ad,Andorra,Europe,Southern Europe,42.5,1.5,🇦🇩,U+1F1E6 U+1F1E9
+15,Austria,AUT,AT,40,43,Vienna,EUR,€,.at,Österreich,Europe,Western Europe,47.33333333,13.33333333,🇦🇹,U+1F1E6 U+1F1F9

Review Comment:
   This new CSV fixture does not appear to be referenced by any regression SQL (e.g. regress/sql/age_load.sql copies the directory but only loads specific files like countries.csv/cities.csv/edges.csv). If it is not needed for tests, consider removing it to avoid adding unused data to the repository.



##########
src/backend/utils/adt/agtype.c:
##########
@@ -11718,8 +11746,43 @@ Datum age_reduce_transfn(PG_FUNCTION_ARGS)
     rc->params[1].isnull = false;
     rc->params[1].execPlan = NULL;
 
-    /* evaluate the fold body for this element */
-    ResetExprContext(rc->econtext);
+    /*
+     * Bind the captured loop-invariant outer values to params 2 .. The values
+     * are pulled from the extras array every row because correlated captures
+     * differ between groups; the per-row deconstruction is done in the
+     * econtext's per-tuple memory (reset above) so it does not leak. A NULL
+     * array element is normalized to agtype 'null' like the accumulator and
+     * element.
+     */
+    if (rc->nparams > 2 && !PG_ARGISNULL(4))
+    {
+        ArrayType *extras_arr = PG_GETARG_ARRAYTYPE_P(4);
+        Oid elemtype = ARR_ELEMTYPE(extras_arr);
+        int16 typlen;
+        bool typbyval;
+        char typalign;
+        Datum *ex_vals;
+        bool *ex_nulls;
+        int ex_n;
+        int i;
+        MemoryContext per_tuple = rc->econtext->ecxt_per_tuple_memory;
+        MemoryContext save = MemoryContextSwitchTo(per_tuple);
+
+        get_typlenbyvalalign(elemtype, &typlen, &typbyval, &typalign);
+        deconstruct_array(extras_arr, elemtype, typlen, typbyval, typalign,
+                          &ex_vals, &ex_nulls, &ex_n);
+
+        for (i = 0; i < ex_n && (2 + i) < rc->nparams; i++)
+        {
+            rc->params[2 + i].value = ex_nulls[i] ? reduce_agtype_null()
+                                                  : ex_vals[i];
+            rc->params[2 + i].isnull = false;
+            rc->params[2 + i].execPlan = NULL;
+        }

Review Comment:
   If the extras array deconstructs to fewer elements than rc->nparams - 2 (e.g. SQL-callable age_reduce() invoked with a varying-length extras array), the remaining PARAM_EXEC slots 2.. stay as their previous values/zero-initialized (ParamExecData.isnull defaults to false). That can yield stale outer values or backend crashes when the fold body reads those params. Bind all captured slots 2.. on every call, filling missing entries with an agtype 'null' Datum.



##########
src/backend/utils/adt/agtype.c:
##########
@@ -11660,6 +11669,20 @@ Datum age_reduce_transfn(PG_FUNCTION_ARGS)
                      errmsg("age_reduce: missing fold expression")));
         }
 
+        /*
+         * The number of captured outer values is fixed for this aggregate
+         * call (the body's structure does not change between rows), so it is
+         * read once here to size the param array. Their values are bound per
+         * row below because a correlated capture changes between groups.
+         */
+        if (!PG_ARGISNULL(4))
+        {
+            ArrayType *extras_arr = PG_GETARG_ARRAYTYPE_P(4);
+
+            n_extras = ArrayGetNItems(ARR_NDIM(extras_arr),
+                                      ARR_DIMS(extras_arr));
+        }

Review Comment:
   age_reduce_transfn() unconditionally reads argument 4 (extras) during one-time initialization. If an older extension definition (or any SQL wrapper) still calls this C symbol with the pre-change 4-argument signature, PG_ARGISNULL(4)/PG_GETARG_ARRAYTYPE_P(4) will read past fcinfo->nargs and can crash the backend. Guard accesses to arg 4 with PG_NARGS() > 4 and treat missing extras as zero captures for backward/upgrade safety.



-- 
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support outer references in reduce() fold bodies [age]

Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.

jrgemignani commented on PR #2448:
URL: https://github.com/apache/age/pull/2448#issuecomment-4814452766

   Resolved all 3 Copilot issues.


-- 
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: commits-unsubscribe@age.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org