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 2025/12/30 22:38:15 UTC
[PR] Optimize id()/start_id()/end_id() with direct column access [age]
jrgemignani opened a new pull request, #2295:
URL: https://github.com/apache/age/pull/2295
NOTE: This PR was created with AI tools and a human
Add optimization to avoid rebuilding full vertex/edge objects when only the id, start_id, or end_id is needed. Instead of calling age_id() on a reconstructed _agtype_build_vertex/_agtype_build_edge, directly access the underlying graphid column and wrap it with graphid_to_agtype().
Implementation:
- Export hidden columns (_age_default_varname_id_*, _age_default_varname_ start_id_*, _age_default_varname_end_id_*) when entities pass between clauses via export_entity_hidden_columns()
- Store Var references (id_var, start_id_var, end_id_var) in transform_entity
- try_optimize_id_funcs() checks for these optimizable patterns:
- Cross-clause: Use stored Var from previous clause export
- Current-clause: Extract graphid Var from entity's build expression
- Validate Var references to prevent stale references after WITH clauses
Optimized patterns:
MATCH (p) RETURN id(p) -- cross-clause in RETURN
MATCH (p) WHERE id(p) > 0 RETURN p -- current-clause in WHERE
MATCH (p) MATCH (q) WHERE id(p) > 0 -- cross-clause in WHERE
Query plan improvement:
Before: Filter: age_id(_agtype_build_vertex(p.id, _label_name(...), p.properties)) After: Filter: graphid_to_agtype(p.id)
All original regression tests passed.
Added additional regression tests.
Files changed:
- src/include/parser/cypher_transform_entity.h: Add id_var, start_id_var, end_id_var, props_var fields to transform_entity struct
- src/backend/parser/cypher_clause.c: Add export_entity_hidden_columns() and integrate with handle_prev_clause()
- src/backend/parser/cypher_expr.c: Add try_optimize_id_funcs(), extract_id_var_from_entity_expr(), find_entity_in_current_cpstate()
- regress/sql/cypher_match.sql: Add 13 WHERE optimization tests
- regress/sql/expr.sql: Add cross-clause optimization tests
- regress/expected/*.out: Update expected output
modified: regress/expected/cypher_match.out
modified: regress/expected/cypher_with.out
modified: regress/sql/cypher_match.sql
modified: regress/sql/cypher_with.sql
modified: src/backend/parser/cypher_clause.c
modified: src/backend/parser/cypher_expr.c
modified: src/backend/parser/cypher_transform_entity.c
modified: src/include/parser/cypher_transform_entity.h
--
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] Optimize id()/start_id()/end_id() with direct column access [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2295:
URL: https://github.com/apache/age/pull/2295#discussion_r2654108103
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,273 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * Export hidden columns for entities in the current clause.
+ *
+ * For each named vertex/edge entity, we export its id (and for edges:
+ * start_id, end_id) as target entries. These can be referenced by
+ * subsequent clauses without rebuilding the full vertex/edge.
+ *
+ * This function handles two cases:
+ * 1. Entities declared in the current clause: Extract id from _agtype_build_* args
+ * 2. Entities from previous clauses (with id_var set): Re-export using their Vars
+ *
+ * The column names use the AGE_VARNAME prefix pattern (AGE_DEFAULT_VARNAME_PREFIX)
+ * which makes them hidden from SELECT * output. The expand_star() function in
+ * cypher_item.c filters out columns with this prefix.
+ *
+ * Note: We use resjunk=false so these columns are included in the subquery RTE
+ * when PostgreSQL builds the column list via addRangeTableEntryForSubquery().
+ * If resjunk=true, PostgreSQL would skip them entirely.
+ */
+static void export_entity_hidden_columns(cypher_parsestate *cpstate,
+ Query *query)
+{
+ ListCell *lc;
+ int resno;
+ Oid graphid_to_agtype_oid;
+ List *exported_names = NIL; /* Track entity names already exported */
+
+ /* Get the graphid_to_agtype function OID for id conversions */
+ graphid_to_agtype_oid = get_ag_func_oid("graphid_to_agtype", 1, GRAPHIDOID);
+
+ /* find the next resno for target entries */
+ resno = list_length(query->targetList) + 1;
+
+ foreach (lc, cpstate->entities)
+ {
+ transform_entity *entity = lfirst(lc);
+ char *entity_name;
+ char *col_name;
+ TargetEntry *te;
+ ListCell *lc2;
+ bool already_exported;
+
+ /* skip entities without names - they aren't referenced */
+ entity_name = get_entity_name(entity);
+ if (entity_name == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * Skip if we've already exported columns for an entity with this name.
+ * This can happen when the same variable appears multiple times in a
+ * pattern, e.g., MATCH (a)-[]->(), ()-[]->(a)
+ */
+ already_exported = false;
+ foreach (lc2, exported_names)
+ {
+ if (strcmp((char *)lfirst(lc2), entity_name) == 0)
+ {
+ already_exported = true;
+ break;
+ }
+ }
+ if (already_exported)
+ {
+ continue;
+ }
+ exported_names = lappend(exported_names, entity_name);
+
+ /* skip path entities - they don't have id/start_id/end_id */
+ if (entity->type == ENT_PATH)
+ {
+ continue;
+ }
+
+ /* skip VLE edges for now - they have different structure */
+ if (entity->type == ENT_VLE_EDGE)
+ {
+ continue;
+ }
+
+ /*
+ * Only export hidden columns for entities declared in the current clause.
+ * Entities from previous clauses already have their id_var/start_id_var/end_id_var
+ * pointing to the subquery's hidden columns from when they were exported.
+ */
+ if (!entity->declared_in_current_clause)
+ {
+ continue;
+ }
+
+ /* Extract id from the entity's FuncExpr */
+ {
+ Expr *expr = entity->expr;
+ FuncExpr *func_expr;
+ Node *id_node;
+ Node *start_id_node;
+ Node *end_id_node;
+ FuncExpr *id_agtype_expr;
+ FuncExpr *start_id_agtype_expr;
+ FuncExpr *end_id_agtype_expr;
+
+ if (expr == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * The expr must be a FuncExpr (_agtype_build_vertex or _agtype_build_edge)
+ * for entities declared in the current clause.
+ */
+ if (!IsA(expr, FuncExpr))
+ {
+ continue;
+ }
+
+ func_expr = (FuncExpr *)expr;
+
+ /*
+ * For vertices: _agtype_build_vertex(id, label_name, properties)
+ * For edges: _agtype_build_edge(id, start_id, end_id, label_name, properties)
+ */
+ if (entity->type == ENT_VERTEX)
+ {
+ /* vertex: args are (id, label_name, properties) */
+ if (list_length(func_expr->args) < 3)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = NULL;
+ end_id_node = NULL;
+ }
+ else if (entity->type == ENT_EDGE)
+ {
+ /* edge: args are (id, start_id, end_id, label_name, properties) */
+ if (list_length(func_expr->args) < 5)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = (Node *)lsecond(func_expr->args);
+ end_id_node = (Node *)lthird(func_expr->args);
+ }
+ else
+ {
+ continue;
+ }
+
+ /*
+ * Create target entries for id (as agtype).
+ * We wrap id in graphid_to_agtype() since age_id() returns agtype.
+ */
+ id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(id_node)), InvalidOid,
+ InvalidOid, COERCE_EXPLICIT_CALL);
+
+ col_name = psprintf("%s%s", AGE_VARNAME_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ /* For edges, also export start_id and end_id */
+ if (entity->type == ENT_EDGE)
+ {
+ start_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(start_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_START_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)start_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ end_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(end_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_END_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)end_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
Review Comment:
Memory leak: col_name allocated with psprintf is never freed. The col_name variable is assigned three times (lines 6587, 6598, 6606) without freeing the previous allocations. Each psprintf call allocates memory that should be freed with pfree. Add pfree(col_name) after using it in makeTargetEntry for each assignment.
##########
src/backend/parser/cypher_expr.c:
##########
@@ -1975,6 +1976,343 @@ static bool function_exists(char *funcname, char *extension)
return found;
}
+/*
+ * Try to optimize id(), start_id(), end_id() function calls.
+ *
+ * When the argument is a variable (vertex or edge) that came from a previous
+ * clause, we may have exposed column Vars (id_var, start_id_var, end_id_var)
+ * that can be used directly instead of calling the function on the full
+ * vertex/edge expression.
+ *
+ * This optimization avoids the expensive reconstruction of the vertex/edge
+ * object just to extract a single field like id.
+ *
+ * Returns NULL if optimization is not possible; otherwise returns the
+ * optimized expression.
+ */
+/*
+ * Helper function to find an entity only in the current cpstate's entities list,
+ * not walking up to parent parsestates. This is important for the optimization
+ * because the entity's Var fields (id_var, etc.) are only valid in the context
+ * where they were set.
+ */
Review Comment:
Duplicate function documentation block. There are two consecutive comment blocks (lines 1979-1992 and 1993-1998) before the find_entity_in_current_cpstate function. The first block appears to be intended for try_optimize_id_funcs (which is declared on line 96 and implemented on line 2149), but is placed in the wrong location. Move the first comment block to line 2149 before the try_optimize_id_funcs implementation.
```suggestion
* Helper function used when optimizing id(), start_id(), end_id() function
* calls.
*
* When the argument to those functions is a variable (vertex or edge) that
* came from a previous clause, we may have exposed column Vars (id_var,
* start_id_var, end_id_var) that can be used directly instead of calling the
* function on the full vertex/edge expression. This function helps that
* optimization by finding an entity only in the current cpstate's entities
* list, not walking up to parent parsestates. This is important because the
* entity's Var fields (id_var, etc.) are only valid in the context where they
* were set.
*/
```
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,273 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * Export hidden columns for entities in the current clause.
+ *
+ * For each named vertex/edge entity, we export its id (and for edges:
+ * start_id, end_id) as target entries. These can be referenced by
+ * subsequent clauses without rebuilding the full vertex/edge.
+ *
+ * This function handles two cases:
+ * 1. Entities declared in the current clause: Extract id from _agtype_build_* args
+ * 2. Entities from previous clauses (with id_var set): Re-export using their Vars
Review Comment:
Misleading documentation comment. The comment at lines 6437-6439 mentions "This function handles two cases" including "2. Entities from previous clauses (with id_var set): Re-export using their Vars", but the actual code at lines 6516-6519 skips entities from previous clauses entirely with the condition "if (!entity->declared_in_current_clause) continue;". The function only exports hidden columns for entities declared in the current clause. Update the comment to remove case 2 or clarify that case 2 is intentionally skipped.
```suggestion
* For each named vertex/edge entity declared in the current clause, we export
* its id (and for edges: start_id, end_id) as target entries. These can be
* referenced by subsequent clauses without rebuilding the full vertex/edge.
*
* Only entities marked as declared in the current clause are processed; entities
* originating from previous clauses (even if they have id_var set) are
* intentionally skipped by this function.
```
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,273 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * Export hidden columns for entities in the current clause.
+ *
+ * For each named vertex/edge entity, we export its id (and for edges:
+ * start_id, end_id) as target entries. These can be referenced by
+ * subsequent clauses without rebuilding the full vertex/edge.
+ *
+ * This function handles two cases:
+ * 1. Entities declared in the current clause: Extract id from _agtype_build_* args
+ * 2. Entities from previous clauses (with id_var set): Re-export using their Vars
+ *
+ * The column names use the AGE_VARNAME prefix pattern (AGE_DEFAULT_VARNAME_PREFIX)
+ * which makes them hidden from SELECT * output. The expand_star() function in
+ * cypher_item.c filters out columns with this prefix.
+ *
+ * Note: We use resjunk=false so these columns are included in the subquery RTE
+ * when PostgreSQL builds the column list via addRangeTableEntryForSubquery().
+ * If resjunk=true, PostgreSQL would skip them entirely.
+ */
+static void export_entity_hidden_columns(cypher_parsestate *cpstate,
+ Query *query)
+{
+ ListCell *lc;
+ int resno;
+ Oid graphid_to_agtype_oid;
+ List *exported_names = NIL; /* Track entity names already exported */
+
+ /* Get the graphid_to_agtype function OID for id conversions */
+ graphid_to_agtype_oid = get_ag_func_oid("graphid_to_agtype", 1, GRAPHIDOID);
+
+ /* find the next resno for target entries */
+ resno = list_length(query->targetList) + 1;
+
+ foreach (lc, cpstate->entities)
+ {
+ transform_entity *entity = lfirst(lc);
+ char *entity_name;
+ char *col_name;
+ TargetEntry *te;
+ ListCell *lc2;
+ bool already_exported;
+
+ /* skip entities without names - they aren't referenced */
+ entity_name = get_entity_name(entity);
+ if (entity_name == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * Skip if we've already exported columns for an entity with this name.
+ * This can happen when the same variable appears multiple times in a
+ * pattern, e.g., MATCH (a)-[]->(), ()-[]->(a)
+ */
+ already_exported = false;
+ foreach (lc2, exported_names)
+ {
+ if (strcmp((char *)lfirst(lc2), entity_name) == 0)
+ {
+ already_exported = true;
+ break;
+ }
+ }
+ if (already_exported)
+ {
+ continue;
+ }
+ exported_names = lappend(exported_names, entity_name);
Review Comment:
Potential issue with exported_names list. The exported_names list is used to track entity names that have already been exported, but it appends entity_name pointers directly without copying the strings. This could be problematic if entity_name is freed or modified after being added to the list. While get_entity_name likely returns a stable pointer from the entity structure, it would be safer to use pstrdup to copy the string or rely on the fact that entity names are stable for the lifetime of the function execution.
```suggestion
exported_names = lappend(exported_names, pstrdup(entity_name));
```
--
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] Optimize id()/start_id()/end_id() with direct column access [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2295:
URL: https://github.com/apache/age/pull/2295#discussion_r2654166684
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,273 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * Export hidden columns for entities in the current clause.
+ *
+ * For each named vertex/edge entity, we export its id (and for edges:
+ * start_id, end_id) as target entries. These can be referenced by
+ * subsequent clauses without rebuilding the full vertex/edge.
+ *
+ * This function handles two cases:
+ * 1. Entities declared in the current clause: Extract id from _agtype_build_* args
+ * 2. Entities from previous clauses (with id_var set): Re-export using their Vars
+ *
+ * The column names use the AGE_VARNAME prefix pattern (AGE_DEFAULT_VARNAME_PREFIX)
+ * which makes them hidden from SELECT * output. The expand_star() function in
+ * cypher_item.c filters out columns with this prefix.
+ *
+ * Note: We use resjunk=false so these columns are included in the subquery RTE
+ * when PostgreSQL builds the column list via addRangeTableEntryForSubquery().
+ * If resjunk=true, PostgreSQL would skip them entirely.
+ */
+static void export_entity_hidden_columns(cypher_parsestate *cpstate,
+ Query *query)
+{
+ ListCell *lc;
+ int resno;
+ Oid graphid_to_agtype_oid;
+ List *exported_names = NIL; /* Track entity names already exported */
+
+ /* Get the graphid_to_agtype function OID for id conversions */
+ graphid_to_agtype_oid = get_ag_func_oid("graphid_to_agtype", 1, GRAPHIDOID);
+
+ /* find the next resno for target entries */
+ resno = list_length(query->targetList) + 1;
+
+ foreach (lc, cpstate->entities)
+ {
+ transform_entity *entity = lfirst(lc);
+ char *entity_name;
+ char *col_name;
+ TargetEntry *te;
+ ListCell *lc2;
+ bool already_exported;
+
+ /* skip entities without names - they aren't referenced */
+ entity_name = get_entity_name(entity);
+ if (entity_name == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * Skip if we've already exported columns for an entity with this name.
+ * This can happen when the same variable appears multiple times in a
+ * pattern, e.g., MATCH (a)-[]->(), ()-[]->(a)
+ */
+ already_exported = false;
+ foreach (lc2, exported_names)
+ {
+ if (strcmp((char *)lfirst(lc2), entity_name) == 0)
+ {
+ already_exported = true;
+ break;
+ }
+ }
+ if (already_exported)
+ {
+ continue;
+ }
+ exported_names = lappend(exported_names, entity_name);
+
+ /* skip path entities - they don't have id/start_id/end_id */
+ if (entity->type == ENT_PATH)
+ {
+ continue;
+ }
+
+ /* skip VLE edges for now - they have different structure */
+ if (entity->type == ENT_VLE_EDGE)
+ {
+ continue;
+ }
+
+ /*
+ * Only export hidden columns for entities declared in the current clause.
+ * Entities from previous clauses already have their id_var/start_id_var/end_id_var
+ * pointing to the subquery's hidden columns from when they were exported.
+ */
+ if (!entity->declared_in_current_clause)
+ {
+ continue;
+ }
+
+ /* Extract id from the entity's FuncExpr */
+ {
+ Expr *expr = entity->expr;
+ FuncExpr *func_expr;
+ Node *id_node;
+ Node *start_id_node;
+ Node *end_id_node;
+ FuncExpr *id_agtype_expr;
+ FuncExpr *start_id_agtype_expr;
+ FuncExpr *end_id_agtype_expr;
+
+ if (expr == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * The expr must be a FuncExpr (_agtype_build_vertex or _agtype_build_edge)
+ * for entities declared in the current clause.
+ */
+ if (!IsA(expr, FuncExpr))
+ {
+ continue;
+ }
+
+ func_expr = (FuncExpr *)expr;
+
+ /*
+ * For vertices: _agtype_build_vertex(id, label_name, properties)
+ * For edges: _agtype_build_edge(id, start_id, end_id, label_name, properties)
+ */
+ if (entity->type == ENT_VERTEX)
+ {
+ /* vertex: args are (id, label_name, properties) */
+ if (list_length(func_expr->args) < 3)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = NULL;
+ end_id_node = NULL;
+ }
+ else if (entity->type == ENT_EDGE)
+ {
+ /* edge: args are (id, start_id, end_id, label_name, properties) */
+ if (list_length(func_expr->args) < 5)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = (Node *)lsecond(func_expr->args);
+ end_id_node = (Node *)lthird(func_expr->args);
+ }
+ else
+ {
+ continue;
+ }
+
+ /*
+ * Create target entries for id (as agtype).
+ * We wrap id in graphid_to_agtype() since age_id() returns agtype.
+ */
+ id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(id_node)), InvalidOid,
+ InvalidOid, COERCE_EXPLICIT_CALL);
+
+ col_name = psprintf("%s%s", AGE_VARNAME_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ /* For edges, also export start_id and end_id */
+ if (entity->type == ENT_EDGE)
+ {
+ start_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(start_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_START_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)start_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ end_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(end_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_END_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)end_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
Review Comment:
Per AI, but I also know this -
No, this is not a memory leak concern in PostgreSQL's memory context system.
In PostgreSQL, memory is allocated in memory contexts (palloc/psprintf use the current memory context). These contexts are freed in bulk at appropriate times (end of transaction, end of query, etc.).
--
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] Optimize id()/start_id()/end_id() with direct column access [age]
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on PR #2295:
URL: https://github.com/apache/age/pull/2295#issuecomment-4876160774
Closing this as superseded by #2303, which is now in `master`.
The main Cypher accessor optimization here is covered by #2303’s composite vertex/edge representation and direct field access. The remaining graphid-vs-int8 operator work seems like a separate raw-SQL feature rather than something required for the Cypher optimization. If we still want that, it should probably be split into a smaller dedicated PR.
--
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] Optimize id()/start_id()/end_id() with direct column access [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2295:
URL: https://github.com/apache/age/pull/2295#discussion_r2654334472
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,277 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * export_entity_hidden_columns
+ *
+ * Add hidden target entries for entity id columns to the query's target list.
+ *
+ * For each named vertex/edge entity declared in the current clause, this
+ * exports its graphid values (id, and for edges: start_id, end_id) as
+ * additional target entries. These hidden columns allow subsequent clauses
+ * to access id values directly via Var references, avoiding the overhead of
+ * reconstructing the full vertex/edge object.
+ *
+ * The column names use the AGE_DEFAULT_VARNAME_PREFIX pattern (e.g.,
+ * "_age_default_varname_id_<name>") which makes them hidden from user-facing
+ * output. The expand_star() function in cypher_item.c filters out columns
+ * with this prefix from SELECT * results.
+ *
+ * Note: resjunk is set to false so PostgreSQL includes these columns when
+ * building the subquery RTE via addRangeTableEntryForSubquery(). Setting
+ * resjunk=true would cause PostgreSQL to omit them entirely.
+ *
+ * After this function runs, update_entity_vars_from_rte() is called to set
+ * each entity's id_var/start_id_var/end_id_var to reference these new columns.
+ */
+static void export_entity_hidden_columns(cypher_parsestate *cpstate,
+ Query *query)
+{
+ ListCell *lc;
+ int resno;
+ Oid graphid_to_agtype_oid;
+ List *exported_names = NIL; /* Track entity names already exported */
+
+ /* Get the graphid_to_agtype function OID for id conversions */
+ graphid_to_agtype_oid = get_ag_func_oid("graphid_to_agtype", 1, GRAPHIDOID);
+
+ /* find the next resno for target entries */
+ resno = list_length(query->targetList) + 1;
+
+ foreach (lc, cpstate->entities)
+ {
+ transform_entity *entity = lfirst(lc);
+ char *entity_name;
+ char *col_name;
+ TargetEntry *te;
+ ListCell *lc2;
+ bool already_exported;
+
+ /* skip entities without names - they aren't referenced */
+ entity_name = get_entity_name(entity);
+ if (entity_name == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * Skip if we've already exported columns for an entity with this name.
+ * This can happen when the same variable appears multiple times in a
+ * pattern, e.g., MATCH (a)-[]->(), ()-[]->(a)
+ */
+ already_exported = false;
+ foreach (lc2, exported_names)
+ {
+ if (strcmp((char *)lfirst(lc2), entity_name) == 0)
+ {
+ already_exported = true;
+ break;
+ }
+ }
+ if (already_exported)
+ {
+ continue;
+ }
+ exported_names = lappend(exported_names, entity_name);
+
+ /* skip path entities - they don't have id/start_id/end_id */
+ if (entity->type == ENT_PATH)
+ {
+ continue;
+ }
+
+ /* skip VLE edges for now - they have different structure */
+ if (entity->type == ENT_VLE_EDGE)
+ {
+ continue;
+ }
+
+ /*
+ * Only export hidden columns for entities declared in the current clause.
+ * Entities from previous clauses already have their id_var/start_id_var/end_id_var
+ * pointing to the subquery's hidden columns from when they were exported.
+ */
+ if (!entity->declared_in_current_clause)
+ {
+ continue;
+ }
+
+ /* Extract id from the entity's FuncExpr */
+ {
+ Expr *expr = entity->expr;
+ FuncExpr *func_expr;
+ Node *id_node;
+ Node *start_id_node;
+ Node *end_id_node;
+ FuncExpr *id_agtype_expr;
+ FuncExpr *start_id_agtype_expr;
+ FuncExpr *end_id_agtype_expr;
+
+ if (expr == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * The expr must be a FuncExpr (_agtype_build_vertex or _agtype_build_edge)
+ * for entities declared in the current clause.
+ */
+ if (!IsA(expr, FuncExpr))
+ {
+ continue;
+ }
+
+ func_expr = (FuncExpr *)expr;
+
+ /*
+ * For vertices: _agtype_build_vertex(id, label_name, properties)
+ * For edges: _agtype_build_edge(id, start_id, end_id, label_name, properties)
+ */
+ if (entity->type == ENT_VERTEX)
+ {
+ /* vertex: args are (id, label_name, properties) */
+ if (list_length(func_expr->args) < 3)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = NULL;
+ end_id_node = NULL;
+ }
+ else if (entity->type == ENT_EDGE)
+ {
+ /* edge: args are (id, start_id, end_id, label_name, properties) */
+ if (list_length(func_expr->args) < 5)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = (Node *)lsecond(func_expr->args);
+ end_id_node = (Node *)lthird(func_expr->args);
+ }
+ else
+ {
+ continue;
+ }
+
+ /*
+ * Create target entries for id (as agtype).
+ * We wrap id in graphid_to_agtype() since age_id() returns agtype.
+ */
+ id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(id_node)), InvalidOid,
+ InvalidOid, COERCE_EXPLICIT_CALL);
+
+ col_name = psprintf("%s%s", AGE_VARNAME_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ /* For edges, also export start_id and end_id */
+ if (entity->type == ENT_EDGE)
+ {
+ start_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(start_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_START_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)start_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ end_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(end_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_END_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)end_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+ }
+ }
+ }
Review Comment:
The `exported_names` list is allocated but never freed. In PostgreSQL's memory context system, this is typically handled automatically when the memory context is reset, but it's better practice to explicitly free the list at the end of the function to avoid unnecessary memory usage during long-running queries with many clauses. Consider adding `list_free(exported_names);` at the end of the function.
```suggestion
}
/* Free the helper list of exported entity names */
list_free(exported_names);
```
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6410,6 +6427,277 @@ transform_cypher_clause_as_subquery(cypher_parsestate *cpstate,
return pnsi;
}
+/*
+ * export_entity_hidden_columns
+ *
+ * Add hidden target entries for entity id columns to the query's target list.
+ *
+ * For each named vertex/edge entity declared in the current clause, this
+ * exports its graphid values (id, and for edges: start_id, end_id) as
+ * additional target entries. These hidden columns allow subsequent clauses
+ * to access id values directly via Var references, avoiding the overhead of
+ * reconstructing the full vertex/edge object.
+ *
+ * The column names use the AGE_DEFAULT_VARNAME_PREFIX pattern (e.g.,
+ * "_age_default_varname_id_<name>") which makes them hidden from user-facing
+ * output. The expand_star() function in cypher_item.c filters out columns
+ * with this prefix from SELECT * results.
+ *
+ * Note: resjunk is set to false so PostgreSQL includes these columns when
+ * building the subquery RTE via addRangeTableEntryForSubquery(). Setting
+ * resjunk=true would cause PostgreSQL to omit them entirely.
+ *
+ * After this function runs, update_entity_vars_from_rte() is called to set
+ * each entity's id_var/start_id_var/end_id_var to reference these new columns.
+ */
+static void export_entity_hidden_columns(cypher_parsestate *cpstate,
+ Query *query)
+{
+ ListCell *lc;
+ int resno;
+ Oid graphid_to_agtype_oid;
+ List *exported_names = NIL; /* Track entity names already exported */
+
+ /* Get the graphid_to_agtype function OID for id conversions */
+ graphid_to_agtype_oid = get_ag_func_oid("graphid_to_agtype", 1, GRAPHIDOID);
+
+ /* find the next resno for target entries */
+ resno = list_length(query->targetList) + 1;
+
+ foreach (lc, cpstate->entities)
+ {
+ transform_entity *entity = lfirst(lc);
+ char *entity_name;
+ char *col_name;
+ TargetEntry *te;
+ ListCell *lc2;
+ bool already_exported;
+
+ /* skip entities without names - they aren't referenced */
+ entity_name = get_entity_name(entity);
+ if (entity_name == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * Skip if we've already exported columns for an entity with this name.
+ * This can happen when the same variable appears multiple times in a
+ * pattern, e.g., MATCH (a)-[]->(), ()-[]->(a)
+ */
+ already_exported = false;
+ foreach (lc2, exported_names)
+ {
+ if (strcmp((char *)lfirst(lc2), entity_name) == 0)
+ {
+ already_exported = true;
+ break;
+ }
+ }
+ if (already_exported)
+ {
+ continue;
+ }
+ exported_names = lappend(exported_names, entity_name);
+
+ /* skip path entities - they don't have id/start_id/end_id */
+ if (entity->type == ENT_PATH)
+ {
+ continue;
+ }
+
+ /* skip VLE edges for now - they have different structure */
+ if (entity->type == ENT_VLE_EDGE)
+ {
+ continue;
+ }
+
+ /*
+ * Only export hidden columns for entities declared in the current clause.
+ * Entities from previous clauses already have their id_var/start_id_var/end_id_var
+ * pointing to the subquery's hidden columns from when they were exported.
+ */
+ if (!entity->declared_in_current_clause)
+ {
+ continue;
+ }
+
+ /* Extract id from the entity's FuncExpr */
+ {
+ Expr *expr = entity->expr;
+ FuncExpr *func_expr;
+ Node *id_node;
+ Node *start_id_node;
+ Node *end_id_node;
+ FuncExpr *id_agtype_expr;
+ FuncExpr *start_id_agtype_expr;
+ FuncExpr *end_id_agtype_expr;
+
+ if (expr == NULL)
+ {
+ continue;
+ }
+
+ /*
+ * The expr must be a FuncExpr (_agtype_build_vertex or _agtype_build_edge)
+ * for entities declared in the current clause.
+ */
+ if (!IsA(expr, FuncExpr))
+ {
+ continue;
+ }
+
+ func_expr = (FuncExpr *)expr;
+
+ /*
+ * For vertices: _agtype_build_vertex(id, label_name, properties)
+ * For edges: _agtype_build_edge(id, start_id, end_id, label_name, properties)
+ */
+ if (entity->type == ENT_VERTEX)
+ {
+ /* vertex: args are (id, label_name, properties) */
+ if (list_length(func_expr->args) < 3)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = NULL;
+ end_id_node = NULL;
+ }
+ else if (entity->type == ENT_EDGE)
+ {
+ /* edge: args are (id, start_id, end_id, label_name, properties) */
+ if (list_length(func_expr->args) < 5)
+ {
+ continue;
+ }
+ id_node = (Node *)linitial(func_expr->args);
+ start_id_node = (Node *)lsecond(func_expr->args);
+ end_id_node = (Node *)lthird(func_expr->args);
+ }
+ else
+ {
+ continue;
+ }
+
+ /*
+ * Create target entries for id (as agtype).
+ * We wrap id in graphid_to_agtype() since age_id() returns agtype.
+ */
+ id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(id_node)), InvalidOid,
+ InvalidOid, COERCE_EXPLICIT_CALL);
+
+ col_name = psprintf("%s%s", AGE_VARNAME_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ /* For edges, also export start_id and end_id */
+ if (entity->type == ENT_EDGE)
+ {
+ start_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(start_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_START_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)start_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+
+ end_id_agtype_expr = makeFuncExpr(graphid_to_agtype_oid, AGTYPEOID,
+ list_make1(copyObject(end_id_node)),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ col_name = psprintf("%s%s", AGE_VARNAME_END_ID_PREFIX, entity_name);
+ te = makeTargetEntry((Expr *)end_id_agtype_expr, resno++, col_name, false);
+ query->targetList = lappend(query->targetList, te);
+ }
Review Comment:
Memory leak: `col_name` is allocated with `psprintf` at line 6591 but is not freed before being reassigned at line 6602. The first allocation will be leaked. The variable should be freed with `pfree(col_name)` before each subsequent `psprintf` assignment. The same issue occurs at line 6610 where `col_name` from line 6602 is not freed before reassignment.
--
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] Optimize id()/start_id()/end_id() with direct column access [age]
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed closed pull request #2295: Optimize id()/start_id()/end_id() with direct column access
URL: https://github.com/apache/age/pull/2295
--
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