Copilot commented on code in PR #2302:
URL: https://github.com/apache/age/pull/2302#discussion_r2678094803
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5485,10 +5493,14 @@ Datum age_end_id(PG_FUNCTION_ARGS)
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("end_id() argument must be an edge or null")));
- agtv_result = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_object, "end_id");
+ /*
+ * Direct field access optimization: end_id is at index 2 for edge
+ * objects due to key length sorting (id=0, label=1, end_id=2).
+ */
Review Comment:
The direct field access optimization assumes edges always have exactly 5
fields. Consider adding an assertion to verify num_pairs equals EDGE_NUM_FIELDS
before accessing fields directly. This would help catch potential data
corruption or unexpected edge structures.
```suggestion
*/
Assert(agtv_object->val.object.num_pairs == EDGE_NUM_FIELDS);
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1597,7 +1795,7 @@ void agtype_hash_scalar_value_extended(const agtype_value
*scalar_val,
case AGTV_VERTEX:
{
graphid id;
- agtype_value *id_agt = GET_AGTYPE_VALUE_OBJECT_VALUE(scalar_val, "id");
+ agtype_value *id_agt = AGTYPE_VERTEX_GET_ID(scalar_val);
Review Comment:
The direct field access optimization assumes vertices always have an id
field at index 0. Consider adding an assertion to verify num_pairs is at least
1 before accessing pairs[0]. This would help catch potential data corruption or
unexpected vertex structures.
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1606,7 +1804,7 @@ void agtype_hash_scalar_value_extended(const agtype_value
*scalar_val,
case AGTV_EDGE:
{
graphid id;
Review Comment:
The direct field access optimization assumes edges always have an id field
at index 0. Consider adding an assertion to verify num_pairs is at least 1
before accessing pairs[0]. This would help catch potential data corruption or
unexpected edge structures.
```suggestion
graphid id;
Assert(scalar_val->val.object.num_pairs >= 1);
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5409,10 +5409,14 @@ Datum age_id(PG_FUNCTION_ARGS)
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("id() argument must be a vertex, an edge or
null")));
- agtv_result = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_object, "id");
+ /*
+ * Direct field access optimization: id is always at index 0 for both
+ * vertex and edge objects due to key length sorting.
+ */
Review Comment:
The direct field access optimization assumes that vertices always have
exactly 3 fields and edges always have exactly 5 fields. Consider adding
assertions to verify num_pairs before accessing fields directly, similar to
what's done in compare_agtype_scalar_values at lines 2000-2001. This would help
catch potential data corruption or unexpected vertex/edge structures.
```suggestion
* vertex and edge objects due to key length sorting.
*
* To guard this assumption, verify the expected number of fields for
* vertices and edges before doing direct field access.
*/
#ifdef USE_ASSERT_CHECKING
if (agtv_object->type == AGTV_VERTEX)
{
Assert(agtv_object->val.object.num_pairs == 3);
}
else
{
Assert(agtv_object->val.object.num_pairs == 5);
}
#endif
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5447,10 +5451,14 @@ Datum age_start_id(PG_FUNCTION_ARGS)
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("start_id() argument must be an edge or
null")));
- agtv_result = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_object, "start_id");
+ /*
+ * Direct field access optimization: start_id is at index 3 for edge
+ * objects due to key length sorting (id=0, label=1, end_id=2, start_id=3).
+ */
Review Comment:
The direct field access optimization assumes edges always have exactly 5
fields. Consider adding an assertion to verify num_pairs equals EDGE_NUM_FIELDS
before accessing fields directly, similar to what's done in
compare_agtype_scalar_values. This would help catch potential data corruption
or unexpected edge structures.
```suggestion
* objects due to key length sorting (id=0, label=1, end_id=2,
start_id=3).
* Verify that the edge has the expected number of fields before
* accessing them directly.
*/
Assert(agtv_object->val.object.num_pairs == EDGE_NUM_FIELDS);
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -6038,10 +6050,22 @@ Datum age_properties(PG_FUNCTION_ARGS)
errmsg("properties() argument must be a vertex, an
edge or null")));
}
- agtv_result = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_object, "properties");
+ /*
+ * Direct field access optimization: properties is at index 2 for vertex
+ * (id=0, label=1, properties=2) and index 4 for edge (id=0, label=1,
+ * end_id=2, start_id=3, properties=4) due to key length sorting.
+ */
+ if (agtv_object->type == AGTV_VERTEX)
+ {
+ agtv_result = AGTYPE_VERTEX_GET_PROPERTIES(agtv_object);
+ }
+ else
+ {
Review Comment:
The direct field access optimization assumes vertices have exactly 3 fields
and edges have exactly 5 fields. Consider adding assertions to verify num_pairs
before accessing fields directly, similar to what's done in
compare_agtype_scalar_values. This would help catch potential data corruption
or unexpected vertex/edge structures.
```suggestion
* end_id=2, start_id=3, properties=4) due to key length sorting.
*
* Verify the expected number of fields before relying on this layout.
*/
if (agtv_object->type == AGTV_VERTEX)
{
Assert(agtv_object->val.object.num_pairs == 3);
agtv_result = AGTYPE_VERTEX_GET_PROPERTIES(agtv_object);
}
else
{
Assert(agtv_object->val.object.num_pairs == 5);
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -7170,8 +7194,11 @@ Datum age_label(PG_FUNCTION_ARGS)
}
- /* extract the label agtype value from the vertex or edge */
- label = GET_AGTYPE_VALUE_OBJECT_VALUE(agtv_value, "label");
+ /*
+ * Direct field access optimization: label is at index 1 for both vertex
+ * and edge objects due to key length sorting (id=0, label=1, ...).
+ */
+ label = AGTYPE_VERTEX_GET_LABEL(agtv_value);
Review Comment:
Using AGTYPE_VERTEX_GET_LABEL for both vertices and edges is correct since
label is at index 1 for both types, but it would be more explicit to use
AGTYPE_EDGE_GET_LABEL when agtv_value->type is AGTV_EDGE. Consider adding a
conditional to improve code clarity, similar to the age_properties()
implementation.
```suggestion
if (agtv_value->type == AGTV_VERTEX)
label = AGTYPE_VERTEX_GET_LABEL(agtv_value);
else /* agtv_value->type must be AGTV_EDGE due to the check above */
label = AGTYPE_EDGE_GET_LABEL(agtv_value);
```
--
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]
