Copilot commented on code in PR #2359:
URL: https://github.com/apache/age/pull/2359#discussion_r2989912663
##########
src/backend/parser/cypher_clause.c:
##########
@@ -1602,6 +1610,130 @@ static Query
*transform_cypher_list_comprehension(cypher_parsestate *cpstate,
return query;
}
+/*
+ * Transform a cypher_predicate_function node into a query tree.
+ *
+ * Similar to list comprehension, but generates:
+ * all() -> SELECT 1 FROM unnest(list) AS x WHERE NOT predicate
+ * any() -> SELECT 1 FROM unnest(list) AS x WHERE predicate
+ * none() -> SELECT 1 FROM unnest(list) AS x WHERE predicate
+ * single() -> SELECT count(*) FROM unnest(list) AS x WHERE predicate
+ *
+ * The grammar layer wraps the SubLink in EXISTS/NOT EXISTS or = 1
+ * to produce the final boolean result.
+ */
+static Query *transform_cypher_predicate_function(cypher_parsestate *cpstate,
+ cypher_clause *clause)
+{
+ Query *query;
+ RangeFunction *rf;
+ cypher_predicate_function *pred_func;
+ FuncCall *func_call;
+ Node *qual, *n;
+ RangeTblEntry *rte = NULL;
+ int rtindex;
+ List *namespace = NULL;
+ TargetEntry *te;
+ cypher_parsestate *child_cpstate = make_cypher_parsestate(cpstate);
+ ParseState *child_pstate = (ParseState *) child_cpstate;
+
+ pred_func = (cypher_predicate_function *) clause->self;
+
+ query = makeNode(Query);
+ query->commandType = CMD_SELECT;
+
+ /* FROM unnest(expr) AS varname */
+ func_call = makeFuncCall(list_make1(makeString("unnest")),
+ list_make1(pred_func->expr),
+ COERCE_SQL_SYNTAX, -1);
+
+ rf = makeNode(RangeFunction);
+ rf->lateral = false;
+ rf->ordinality = false;
+ rf->is_rowsfrom = false;
+ rf->functions = list_make1(list_make2((Node *) func_call, NIL));
+ rf->alias = makeAlias(pred_func->varname, NIL);
+ rf->coldeflist = NIL;
+
+ n = transform_from_clause_item(child_cpstate, (Node *) rf,
+ &rte, &rtindex, &namespace);
+ checkNameSpaceConflicts(child_pstate, child_pstate->p_namespace,
namespace);
+ child_pstate->p_joinlist = lappend(child_pstate->p_joinlist, n);
+ child_pstate->p_namespace = list_concat(child_pstate->p_namespace,
+ namespace);
+
+ /* make all namespace items unconditionally visible */
+ setNamespaceLateralState(child_pstate->p_namespace, false, true);
+
+ /* WHERE clause -- transform the predicate */
+ qual = transform_cypher_expr(child_cpstate, pred_func->where,
+ EXPR_KIND_WHERE);
+ if (qual)
+ {
+ qual = coerce_to_boolean(child_pstate, qual, "WHERE");
+ }
Review Comment:
The current SQL shape (unnest(...) with the predicate in the WHERE clause)
collapses NULL predicate results to “no row”, which changes Cypher semantics.
Examples: `any(x IN [NULL] WHERE x > 0)` and `all(x IN [1] WHERE null)` should
yield SQL NULL (unknown), but `WHERE NULL` filters the row out so `EXISTS`/`NOT
EXISTS` will return false/true instead. Similarly, `any(x IN null WHERE ...)`
should produce NULL, but `unnest(NULL::anyarray)` returns 0 rows so the result
becomes false/true. Consider implementing predicate functions using explicit
`predicate IS TRUE` / `predicate IS FALSE` / `predicate IS NULL` checks (and a
`list IS NULL` guard) to preserve three-valued logic, and add regression tests
for these cases.
##########
src/backend/parser/cypher_clause.c:
##########
@@ -1602,6 +1610,130 @@ static Query
*transform_cypher_list_comprehension(cypher_parsestate *cpstate,
return query;
}
+/*
+ * Transform a cypher_predicate_function node into a query tree.
+ *
+ * Similar to list comprehension, but generates:
+ * all() -> SELECT 1 FROM unnest(list) AS x WHERE NOT predicate
+ * any() -> SELECT 1 FROM unnest(list) AS x WHERE predicate
+ * none() -> SELECT 1 FROM unnest(list) AS x WHERE predicate
+ * single() -> SELECT count(*) FROM unnest(list) AS x WHERE predicate
+ *
+ * The grammar layer wraps the SubLink in EXISTS/NOT EXISTS or = 1
+ * to produce the final boolean result.
+ */
+static Query *transform_cypher_predicate_function(cypher_parsestate *cpstate,
+ cypher_clause *clause)
+{
+ Query *query;
+ RangeFunction *rf;
+ cypher_predicate_function *pred_func;
+ FuncCall *func_call;
+ Node *qual, *n;
+ RangeTblEntry *rte = NULL;
+ int rtindex;
+ List *namespace = NULL;
+ TargetEntry *te;
+ cypher_parsestate *child_cpstate = make_cypher_parsestate(cpstate);
+ ParseState *child_pstate = (ParseState *) child_cpstate;
+
+ pred_func = (cypher_predicate_function *) clause->self;
+
+ query = makeNode(Query);
+ query->commandType = CMD_SELECT;
+
+ /* FROM unnest(expr) AS varname */
+ func_call = makeFuncCall(list_make1(makeString("unnest")),
+ list_make1(pred_func->expr),
+ COERCE_SQL_SYNTAX, -1);
+
+ rf = makeNode(RangeFunction);
+ rf->lateral = false;
+ rf->ordinality = false;
+ rf->is_rowsfrom = false;
+ rf->functions = list_make1(list_make2((Node *) func_call, NIL));
+ rf->alias = makeAlias(pred_func->varname, NIL);
+ rf->coldeflist = NIL;
+
+ n = transform_from_clause_item(child_cpstate, (Node *) rf,
+ &rte, &rtindex, &namespace);
+ checkNameSpaceConflicts(child_pstate, child_pstate->p_namespace,
namespace);
+ child_pstate->p_joinlist = lappend(child_pstate->p_joinlist, n);
+ child_pstate->p_namespace = list_concat(child_pstate->p_namespace,
+ namespace);
+
+ /* make all namespace items unconditionally visible */
+ setNamespaceLateralState(child_pstate->p_namespace, false, true);
+
+ /* WHERE clause -- transform the predicate */
+ qual = transform_cypher_expr(child_cpstate, pred_func->where,
+ EXPR_KIND_WHERE);
+ if (qual)
+ {
+ qual = coerce_to_boolean(child_pstate, qual, "WHERE");
+ }
+
+ /*
+ * For all(): negate the predicate so NOT EXISTS(... WHERE NOT pred)
+ * gives the correct "true when all match" semantics.
+ */
+ if (pred_func->kind == CPFK_ALL && qual != NULL)
+ {
+ qual = (Node *)makeBoolExpr(NOT_EXPR, list_make1(qual), -1);
+ }
+
+ if (pred_func->kind == CPFK_SINGLE)
+ {
+ /* SELECT count(*) -- for single() */
+ FuncCall *count_call;
+ Node *count_expr;
+
+ count_call = makeFuncCall(list_make1(makeString("count")),
+ NIL, COERCE_SQL_SYNTAX, -1);
+ count_call->agg_star = true;
+
+ count_expr = transformExpr(child_pstate, (Node *)count_call,
+ EXPR_KIND_SELECT_TARGET);
+
+ te = makeTargetEntry((Expr *)count_expr,
+ (AttrNumber) child_pstate->p_next_resno++,
+ "count", false);
+ }
Review Comment:
`single()` is implemented via `count(*)` over the full `unnest()` result,
which can’t short-circuit and will scan all list elements even after finding 2
matches. To keep `single()` closer to the intended “early exit” behavior,
consider counting over a `LIMIT 2` subquery (or an equivalent EXISTS-based
approach) so evaluation can stop once more than one match is found.
##########
src/backend/parser/cypher_gram.y:
##########
@@ -3268,6 +3295,35 @@ static cypher_relationship *build_VLE_relation(List
*left_arg,
return cr;
}
+/*
+ * Extract and validate the iterator variable name from a ColumnRef node.
+ * Used by predicate functions (all/any/none/single) which share the
+ * "variable IN list" syntax with list comprehensions.
+ */
+static char *extract_iter_variable_name(Node *var)
+{
+ ColumnRef *cref;
+ String *val;
+
+ if (!IsA(var, ColumnRef))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("syntax error at or near IN")));
+ }
+
+ cref = (ColumnRef *)var;
+ val = linitial(cref->fields);
+ if (!IsA(val, String))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid iterator variable name")));
+ }
+
+ return val->sval;
Review Comment:
`extract_iter_variable_name()` only checks that the iterator is a
`ColumnRef` whose first field is a `String`, but it doesn’t verify the
`ColumnRef` is unqualified (single field). As written, `all(x.y IN list WHERE
...)` would be accepted and silently treated as iterator `x`, ignoring `.y`.
Consider enforcing `list_length(cref->fields) == 1` (and erroring otherwise) so
only valid iterator variables are accepted.
##########
regress/sql/predicate_functions.sql:
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+LOAD 'age';
+SET search_path TO ag_catalog;
+
+SELECT create_graph('predicate_functions');
+
+--
+-- all() predicate function
+--
+-- all elements satisfy predicate -> true
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN all(x IN [1, 2, 3] WHERE x > 0)
+$$) AS (result agtype);
+
+-- not all elements satisfy predicate -> false
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN all(x IN [1, 2, 3] WHERE x > 1)
+$$) AS (result agtype);
+
+-- empty list -> true (vacuous truth)
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN all(x IN [] WHERE x > 0)
+$$) AS (result agtype);
+
+--
+-- any() predicate function
+--
+-- at least one element satisfies -> true
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN any(x IN [1, 2, 3] WHERE x > 2)
+$$) AS (result agtype);
+
+-- no element satisfies -> false
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN any(x IN [1, 2, 3] WHERE x > 5)
+$$) AS (result agtype);
+
+-- empty list -> false
+SELECT * FROM cypher('predicate_functions', $$
+ RETURN any(x IN [] WHERE x > 0)
+$$) AS (result agtype);
+
Review Comment:
The regression tests cover empty-list edge cases, but they don’t cover NULL
propagation cases that this repo already treats as SQL NULL (e.g., `true IN
NULL` in `expr.sql`). Please add tests for (1) NULL list input
(`all/any/none/single(x IN null WHERE ...)`) and (2) predicates that evaluate
to NULL for some elements (e.g., list contains `null`, or predicate returns
`null`) to lock in correct three-valued semantics.
--
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]
