Copilot commented on code in PR #2420:
URL: https://github.com/apache/age/pull/2420#discussion_r3171526642
##########
src/backend/parser/cypher_clause.c:
##########
@@ -3998,56 +3994,86 @@ static List
*make_join_condition_for_edge(cypher_parsestate *cpstate,
}
/*
- * If the previous node and the next node are in the join tree, we need
- * to create the age_match_vle_terminal_edge to compare the vle
returned
- * results against the two nodes.
+ * S5: if the previous and next nodes are both in the join tree,
+ * emit two graphid equality A_Exprs:
+ * <vle_alias>.start_id = prev_node.id
+ * <vle_alias>.end_id = next_node.id
+ * This replaces the historical per-row
+ * age_match_vle_terminal_edge(prev.id, next.id, edges)
+ * function call with plain integer (int8) equality quals on the
+ * SRF's S4 output columns. The planner can now drive the join
+ * directly on these keys (HashJoin hash keys, NestLoop index
+ * conditions where indexed).
*/
if (prev_node->in_join_tree)
{
- func_name = makeString("age_match_vle_terminal_edge");
- qualified_func_name = list_make2(ag_catalog, func_name);
+ ColumnRef *cr_start;
+ ColumnRef *cr_end;
+ A_Expr *eq_start;
+ A_Expr *eq_end;
- /*
- * Get the vertex's id and pass to the function. Pass in NULL
- * otherwise.
- */
- left_id = (Node *)make_qual(cpstate, prev_node, "id");
+ Assert(entity->vle_alias != NULL);
+
+ cr_start = makeNode(ColumnRef);
+ cr_start->fields = list_make2(makeString(entity->vle_alias),
+ makeString("start_id"));
+ cr_start->location = -1;
+
+ cr_end = makeNode(ColumnRef);
+ cr_end->fields = list_make2(makeString(entity->vle_alias),
+ makeString("end_id"));
+ cr_end->location = -1;
+
+ left_id = (Node *)make_qual(cpstate, prev_node, "id");
right_id = (Node *)make_qual(cpstate, next_node, "id");
- /* create the argument list */
- args = list_make3(left_id, right_id, entity->expr);
+ eq_start = makeSimpleA_Expr(AEXPR_OP, "=",
+ (Node *)cr_start, left_id, -1);
+ eq_end = makeSimpleA_Expr(AEXPR_OP, "=",
+ (Node *)cr_end, right_id, -1);
- /* add to quals */
- quals = lappend(quals, makeFuncCall(qualified_func_name, args,
- COERCE_EXPLICIT_CALL, -1));
+ quals = lappend(quals, eq_start);
+ quals = lappend(quals, eq_end);
}
/*
- * When the previous node is not in the join tree, but there is a vle
- * edge before that join, then we need to compare this vle's start node
- * against the previous vle's end node. No need to check the next edge,
- * because that would be redundant.
+ * S6: when the previous node is not in the join tree but there is
+ * a vle edge before that join, emit a single graphid equality
+ * connecting the two VLE SRFs:
+ *
+ * prev_vle.end_id = this_vle.start_id
+ *
+ * This replaces the per-row age_match_two_vle_edges(prev, this)
+ * function call with a plain int8 equality on the S4 scalar
+ * output columns of both age_vle SRFs. No detoasting of either
+ * VLE_path_container is needed.
*/
if (!prev_node->in_join_tree &&
prev_edge != NULL &&
prev_edge->type == ENT_VLE_EDGE)
{
- List *qualified_name;
- String *match_qual;
- FuncCall *fc;
+ ColumnRef *cr_prev_end;
+ ColumnRef *cr_this_start;
+ A_Expr *eq_chain;
- match_qual = makeString("age_match_two_vle_edges");
+ Assert(prev_edge->vle_alias != NULL);
+ Assert(entity->vle_alias != NULL);
- /* make the qualified function name */
- qualified_name = list_make2(ag_catalog, match_qual);
+ cr_prev_end = makeNode(ColumnRef);
+ cr_prev_end->fields = list_make2(makeString(prev_edge->vle_alias),
+ makeString("end_id"));
+ cr_prev_end->location = -1;
- /* make the args */
- args = list_make2(prev_edge->expr, entity->expr);
+ cr_this_start = makeNode(ColumnRef);
+ cr_this_start->fields = list_make2(makeString(entity->vle_alias),
+ makeString("start_id"));
Review Comment:
`prev_edge->vle_alias` / `entity->vle_alias` are validated only via
`Assert()`. If asserts are compiled out, a NULL alias will cause invalid
ColumnRef construction and can crash the backend. Please add a non-asserting
runtime check (ereport(ERROR, ...)) before using these pointers.
##########
src/backend/parser/cypher_clause.c:
##########
@@ -3998,56 +3994,86 @@ static List
*make_join_condition_for_edge(cypher_parsestate *cpstate,
}
/*
- * If the previous node and the next node are in the join tree, we need
- * to create the age_match_vle_terminal_edge to compare the vle
returned
- * results against the two nodes.
+ * S5: if the previous and next nodes are both in the join tree,
+ * emit two graphid equality A_Exprs:
+ * <vle_alias>.start_id = prev_node.id
+ * <vle_alias>.end_id = next_node.id
+ * This replaces the historical per-row
+ * age_match_vle_terminal_edge(prev.id, next.id, edges)
+ * function call with plain integer (int8) equality quals on the
+ * SRF's S4 output columns. The planner can now drive the join
+ * directly on these keys (HashJoin hash keys, NestLoop index
+ * conditions where indexed).
*/
if (prev_node->in_join_tree)
{
- func_name = makeString("age_match_vle_terminal_edge");
- qualified_func_name = list_make2(ag_catalog, func_name);
+ ColumnRef *cr_start;
+ ColumnRef *cr_end;
+ A_Expr *eq_start;
+ A_Expr *eq_end;
- /*
- * Get the vertex's id and pass to the function. Pass in NULL
- * otherwise.
- */
- left_id = (Node *)make_qual(cpstate, prev_node, "id");
+ Assert(entity->vle_alias != NULL);
+
+ cr_start = makeNode(ColumnRef);
+ cr_start->fields = list_make2(makeString(entity->vle_alias),
+ makeString("start_id"));
+ cr_start->location = -1;
+
+ cr_end = makeNode(ColumnRef);
+ cr_end->fields = list_make2(makeString(entity->vle_alias),
+ makeString("end_id"));
+ cr_end->location = -1;
Review Comment:
`entity->vle_alias` is only protected by `Assert()`. In production builds
where asserts are disabled, a missing alias would lead to NULL being passed
into `makeString()` and likely a crash while building the ColumnRef. Please
replace the Assert with a runtime check (ereport(ERROR, ...) with a useful
message) before constructing `cr_start/cr_end`.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -140,13 +140,34 @@ typedef struct VLE_local_context
* structure is set up to contains a BINARY container that can be accessed by
* functions that need to process the path.
*/
+/*
+ * Layout (offsets, with int64 alignment):
+ *
+ * 0: vl_len_[4] varlena length header (int32 + pad)
+ * 4: header AGT_FBINARY | AGT_FBINARY_TYPE_VLE_PATH
+ * 8: graph_oid source graph oid
+ * 12: (4 bytes pad) int64 alignment
+ * 16: graphid_array_size number of graphids in the path
+ * 24: container_size_bytes total bytes of this container
+ * 32: start_vid redundant cache of graphid_array[0]
+ * 40: end_vid redundant cache of
+ * graphid_array[graphid_array_size - 1]
+ * 48: graphid_array_data flexible array start
+ *
+ * start_vid / end_vid are populated whenever the container is built and let
+ * downstream consumers (the age_vle SRF's start_id/end_id output columns)
+ * read the join endpoints without traversing the (potentially toasted)
+ * variadic payload.
+ */
typedef struct VLE_path_container
{
char vl_len_[4]; /* Do not touch this field! */
uint32 header;
uint32 graph_oid;
int64 graphid_array_size;
int64 container_size_bytes;
+ graphid start_vid;
+ graphid end_vid;
graphid graphid_array_data;
} VLE_path_container;
Review Comment:
Adding `start_vid/end_vid` changes the in-memory/on-disk layout of the
`AGT_FBINARY_TYPE_VLE_PATH` blob, but the type tag
(`AGT_FBINARY_TYPE_VLE_PATH`) and access macro
(`GET_GRAPHID_ARRAY_FROM_CONTAINER`) remain unchanged. Any VLE containers
persisted from older versions (e.g., stored as agtype and later passed to
`age_materialize_vle_path/edges` or `agtype_build_path`) will be misinterpreted
with the new offsets. Consider versioning the blob format (new binary flag or a
version field) and/or adding backward-compatible decoding based on
`VARSIZE`/`container_size_bytes`.
--
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]
