Copilot commented on code in PR #2303:
URL: https://github.com/apache/age/pull/2303#discussion_r2678745809
##########
regress/sql/expr.sql:
##########
@@ -3712,9 +3712,360 @@ $$) AS (out agtype);
SELECT * FROM create_graph('issue_2289');
SELECT * FROM cypher('issue_2289', $$ RETURN (1 IN []) AS v $$) AS (v agtype);
+--
+-- Accessor functions and properties extraction
+-- without _agtype_build.. functions
+--
+SELECT * FROM create_graph('accessor_opt');
+
+SELECT * FROM cypher('accessor_opt', $$
+ CREATE (a:Person {name: 'Alice', age: 30})-[r:KNOWS {since:
2020}]->(b:Person {name: 'Bob', age: 25})
+$$) AS (a agtype);
+
+--
+-- Vertex accessor tests
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN id(n)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN properties(n)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ MATCH (n:Person)
+ RETURN label(n)
+$$) AS (plan agtype);
+
+-- should use n.properties in _agtype_access_operator
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN n.name, n.age
+$$) AS (a agtype, b agtype);
+
+--
+-- Edge accessor tests
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ MATCH ()-[r:KNOWS]->()
+ RETURN type(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN start_id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN end_id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN properties(r)
+$$) AS (plan agtype);
+
+-- should use r.properties in _agtype_access_operator
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN r.since
+$$) AS (plan agtype);
+
+--
+-- Multiple accessors in same query
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN id(n) properties(n), n.name
+$$) AS (a agtype, c agtype, d agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN id(r), start_id(r), end_id(r), properties(r)
+$$) AS (a agtype, c agtype, d agtype, e agtype);
+
+--
+-- Accessors in WHERE clause
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ WHERE id(n) > 0
+ RETURN n.name
+$$) AS (plan agtype);
+
+-- Compare two node ids
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (a:Person)-[r:KNOWS]->(b:Person)
+ WHERE id(a) < id(b)
+ RETURN a.name, b.name
+$$) AS (aname agtype, bname agtype);
+
+-- Compare edge start_id and end_id
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ WHERE start_id(r) <> end_id(r)
+ RETURN id(r)
+$$) AS (rid agtype);
+
+-- Property comparison
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ WHERE n.age >= 25
+ RETURN n.name
+$$) AS (name agtype);
+
+-- Multiple property comparisons
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ WHERE n.age > 20 AND n.name <> 'Unknown'
+ RETURN n.name
+$$) AS (name agtype);
+
+-- Compare properties of two nodes
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (a:Person)-[r:KNOWS]->(b:Person)
+ WHERE a.age > b.age
+ RETURN a.name, b.name
+$$) AS (aname agtype, bname agtype);
+
+-- Compare id with property
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ WHERE id(n) > n.age
+ RETURN n.name
+$$) AS (name agtype);
+
+-- Edge property comparison
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ WHERE r.since > 2015
+ RETURN r.since
+$$) AS (since agtype);
+
+-- IS NOT NULL on accessor
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ WHERE properties(n) IS NOT NULL
+ RETURN n.name
+$$) AS (name agtype);
+
+-- label() comparison
+SELECT * FROM cypher('accessor_opt', $$
+ MATCH (n)
+ WHERE label(n) = 'Person'
+ RETURN n.name
+$$) AS (name agtype);
+
+-- type() comparison on edge
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r]->()
+ WHERE type(r) = 'KNOWS'
+ RETURN id(r)
+$$) AS (rid agtype);
+
+--
+-- Accessors in ORDER BY
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN n.name
+ ORDER BY n.name
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN n.name
+ ORDER BY id(n)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN n.name, n.age
+ ORDER BY n.age, n.name
+$$) AS (name agtype, age agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN r.since
Review Comment:
The ORDER BY clause specifies start_id(r) but only one column is returned
(r.since), yet the AS clause expects two columns (typ agtype, since agtype).
This mismatch will cause a runtime error.
```suggestion
RETURN type(r), r.since
```
##########
regress/sql/expr.sql:
##########
@@ -3712,9 +3712,360 @@ $$) AS (out agtype);
SELECT * FROM create_graph('issue_2289');
SELECT * FROM cypher('issue_2289', $$ RETURN (1 IN []) AS v $$) AS (v agtype);
+--
+-- Accessor functions and properties extraction
+-- without _agtype_build.. functions
+--
+SELECT * FROM create_graph('accessor_opt');
+
+SELECT * FROM cypher('accessor_opt', $$
+ CREATE (a:Person {name: 'Alice', age: 30})-[r:KNOWS {since:
2020}]->(b:Person {name: 'Bob', age: 25})
+$$) AS (a agtype);
+
+--
+-- Vertex accessor tests
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN id(n)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN properties(n)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ MATCH (n:Person)
+ RETURN label(n)
+$$) AS (plan agtype);
+
+-- should use n.properties in _agtype_access_operator
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN n.name, n.age
+$$) AS (a agtype, b agtype);
+
+--
+-- Edge accessor tests
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ MATCH ()-[r:KNOWS]->()
+ RETURN type(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN start_id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN end_id(r)
+$$) AS (plan agtype);
+
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN properties(r)
+$$) AS (plan agtype);
+
+-- should use r.properties in _agtype_access_operator
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH ()-[r:KNOWS]->()
+ RETURN r.since
+$$) AS (plan agtype);
+
+--
+-- Multiple accessors in same query
+--
+SELECT * FROM cypher('accessor_opt', $$
+ EXPLAIN (VERBOSE, COSTS OFF)
+ MATCH (n:Person)
+ RETURN id(n) properties(n), n.name
Review Comment:
Missing comma between the two return values in the AS clause. The test
expects three columns (a, c, d) but the RETURN clause appears to have "id(n)
properties(n)" which is missing a comma between them. This is likely a typo in
the test query itself.
```suggestion
RETURN id(n), properties(n), n.name
```
##########
src/backend/parser/cypher_expr.c:
##########
@@ -2001,6 +2068,145 @@ static bool function_exists(char *funcname, char
*extension)
return found;
}
+/*
+ * Check if function name is an accessor function that can be optimized
+ */
+static bool is_accessor_function(const char *func_name)
+{
+ return (strcasecmp("id", func_name) == 0 ||
+ strcasecmp("properties", func_name) == 0 ||
+ strcasecmp("type", func_name) == 0 ||
+ strcasecmp("label", func_name) == 0 ||
+ strcasecmp("start_id", func_name) == 0 ||
+ strcasecmp("end_id", func_name) == 0 ||
+ strcasecmp("startnode", func_name) == 0 ||
+ strcasecmp("endnode", func_name) == 0);
+}
+
+static Oid get_entity_record_type(Node *node)
+{
+ Oid typeid = InvalidOid;
+
+ if (IsA(node, RowExpr))
+ {
+ typeid = ((RowExpr *)node)->row_typeid;
+ }
+ else if (IsA(node, Var))
+ {
+ typeid = ((Var *)node)->vartype;
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ if (typeid != VERTEXOID && typeid != EDGEOID)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ return typeid;
+}
+
+/*
+ * Optimize accessor functions when called on RECORD entities
+ * Extracts fields directly using FieldSelect instead of building and parsing
agtype
+ */
+static Node *optimize_accessor_function(cypher_parsestate *cpstate,
+ const char *func_name,
+ FuncCall *fn,
+ List *targs)
+{
+ ParseState *pstate = &cpstate->pstate;
+ Node *arg;
+ AttrNumber fieldnum;
+ Oid fieldtype;
+ Node *result;
+ Oid entity_type;
+ bool is_node_func = (strcasecmp(func_name, "startNode") == 0 ||
+ strcasecmp(func_name, "endNode") == 0);
+
+ if (is_node_func)
+ {
+ if (list_length(targs) < 2)
+ return NULL;
+ arg = (Node *)lsecond(targs);
+ }
+ else
+ {
+ if (list_length(targs) < 1)
+ return NULL;
+ arg = (Node *)linitial(targs);
+ }
+
+ if (!is_vertex_or_edge(arg))
+ {
+ return NULL;
+ }
+
+ entity_type = get_entity_record_type(arg);
+ get_record_field_info((char *)func_name, entity_type, &fieldnum,
&fieldtype);
+
+ if (fieldnum != InvalidAttrNumber && fieldtype != InvalidOid)
+ {
+ Node *field = NULL;
+
+ /* Validate RowExpr has enough fields */
+ if (IsA(arg, RowExpr))
+ {
+ RowExpr *re = (RowExpr *)arg;
+ if (list_length(re->args) < fieldnum)
+ {
+ return NULL;
+ }
+
+ /* RowExpr args are 0-indexed, but fieldnum is 1-indexed */
+ field = (Node *)list_nth(re->args, fieldnum - 1);
+ }
+ else if (IsA(arg, Var))
+ {
+ field = (Node *)make_field_select((Var *)arg, fieldnum, fieldtype);
+ }
+
+ /* For startNode/endNode functions on edges, look up the vertex */
+ if (is_node_func && entity_type == EDGEOID)
+ {
+ Oid func_oid;
+ Node *graph_name;
+
+ graph_name = (Node *) makeConst(TEXTOID, -1, InvalidOid, -1,
+
CStringGetTextDatum(cpstate->graph_name),
+ false, false);
+
+ /* Call _get_vertex_by_graphid(graph_name, vertex_id) */
+ func_oid = get_ag_func_oid("_get_vertex_by_graphid", 2, TEXTOID,
GRAPHIDOID);
+ result = (Node *)makeFuncExpr(func_oid, AGTYPEOID,
+ list_make2(graph_name, field),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ }
+ else
+ {
+ result = field;
+ }
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("%s() argument must be an %s or null",
+ func_name,
+ (entity_type == VERTEXOID) ? "edge" : "vertex"),
Review Comment:
The error message incorrectly states that the argument must be an "edge"
when it should be a "vertex". The logic is inverted: when entity_type ==
VERTEXOID, the error says "must be an edge", but accessor functions like
start_id() and end_id() should only work on edges, not vertices.
```suggestion
(entity_type == VERTEXOID) ? "vertex" : "edge"),
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -2308,7 +2356,25 @@ Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
}
id = AG_GETARG_GRAPHID(0);
- label = PG_GETARG_CSTRING(1);
+ label_agtype = AG_GET_ARG_AGTYPE_P(1);
+
+ /* Extract the string from the agtype label */
+ if (!AGT_ROOT_IS_SCALAR(label_agtype))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_vertex() label must be a scalar
string")));
+ }
+
+ label_value = get_ith_agtype_value_from_container(&label_agtype->root, 0);
+ if (label_value->type != AGTV_STRING)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_vertex() label must be a string")));
+ }
+
+ label = pnstrdup(label_value->val.string.val, label_value->val.string.len);
Review Comment:
The pnstrdup allocates memory for the label string, but there's no
corresponding pfree to release this memory. While the label is used briefly in
this function, it should be freed before returning to avoid a memory leak.
```suggestion
label = pnstrdup(label_value->val.string.val,
label_value->val.string.len);
pfree(label);
```
##########
src/backend/parser/cypher_expr.c:
##########
@@ -1161,6 +1162,67 @@ static Node *transform_cypher_map(cypher_parsestate
*cpstate, cypher_map *cm)
return (Node *)fexpr;
}
+/*
+ * Check if a node is a vertex or edge composite type.
+ */
+bool is_vertex_or_edge(Node *node)
+{
+ Oid typeid;
+
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, RowExpr))
+ typeid = ((RowExpr *)node)->row_typeid;
+ else if (IsA(node, Var))
+ typeid = ((Var *)node)->vartype;
+ else
+ return false;
+
+ return (typeid == VERTEXOID || typeid == EDGEOID);
+}
+
+/*
+ * Extract a field from a vertex/edge Var or RowExpr.
+ */
+Node *extract_field_from_record(Node *node, char *field_name)
+{
+ AttrNumber fieldnum;
+ Oid fieldtype;
+ Oid type_id = get_entity_record_type(node);
+
+ get_record_field_info(field_name, type_id,
+ &fieldnum, &fieldtype);
+
+ if (fieldnum == InvalidAttrNumber || fieldtype == InvalidOid)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("fieldname '%s' does not exist for the entity",
field_name)));
+ }
+
+ if (IsA(node, Var))
+ {
+ return (Node *)make_field_select((Var *)node, fieldnum, fieldtype);
+ }
+ else if (IsA(node, RowExpr))
+ {
+ RowExpr *re = (RowExpr *)node;
+
+ if (list_length(re->args) < fieldnum)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("field '%s' does not exist in RowExpr",
field_name)));
+ }
+
Review Comment:
Trailing whitespace at the end of line 1218 should be removed to maintain
consistent code formatting.
```suggestion
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -2381,10 +2450,28 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
if (fcinfo->args[3].isnull)
{
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("_agtype_build_vertex() label cannot be
NULL")));
+ errmsg("_agtype_build_edge() label cannot be NULL")));
}
- label = PG_GETARG_CSTRING(3);
+ label_agtype = AG_GET_ARG_AGTYPE_P(3);
+
+ /* Extract the string from the agtype label */
+ if (!AGT_ROOT_IS_SCALAR(label_agtype))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_edge() label must be a scalar
string")));
+ }
+
+ label_value = get_ith_agtype_value_from_container(&label_agtype->root, 0);
+ if (label_value->type != AGTV_STRING)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_edge() label must be a string")));
+ }
+
+ label = pnstrdup(label_value->val.string.val, label_value->val.string.len);
Review Comment:
Same memory leak issue as in _agtype_build_vertex: pnstrdup allocates memory
for label but it's never freed before returning.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -12138,11 +12342,12 @@ Datum agtype_volatile_wrapper(PG_FUNCTION_ARGS)
agtv_result.type = AGTV_BOOL;
agtv_result.val.boolean = DatumGetBool(arg);
}
- else if (type == INT2OID || type == INT4OID || type == INT8OID)
+ else if (type == INT2OID || type == INT4OID ||
+ type == INT8OID || type == GRAPHIDOID)
{
agtv_result.type = AGTV_INTEGER;
- if (type == INT8OID)
+ if (type == INT8OID || type == GRAPHIDOID)
{
agtv_result.val.int_value = DatumGetInt64(arg);
}
Review Comment:
The function agtype_volatile_wrapper now handles GRAPHIDOID by treating it
the same as INT8OID (converting to AGTV_INTEGER). However, GRAPHIDOID should
potentially be handled differently to preserve its semantic meaning as a graph
identifier rather than just an integer. Consider whether graphid values should
be wrapped in a specific agtype structure to distinguish them from regular
integers.
--
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]
