Copilot commented on code in PR #2411:
URL: https://github.com/apache/age/pull/2411#discussion_r3164572475
##########
src/backend/utils/adt/agtype.c:
##########
@@ -12450,6 +12450,115 @@ Datum age_unnest(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+PG_FUNCTION_INFO_V1(age_unwind);
+/*
+ * Function to convert an agtype array into a set of rows for the Cypher
+ * `UNWIND` clause. Behaves like age_unnest() but, per Neo4j/openCypher
+ * semantics, emits an SQL NULL row for each top-level element that is an
+ * agtype JSON null (AGTV_NULL). This lets null-strict operators such as
+ * count(x) and IS NULL treat `UNWIND [null] AS x` the same as
+ * `WITH null AS x`. Nested nulls inside arrays/objects are preserved as
+ * agtype-null so container semantics are unchanged. See issue #2383.
+ */
+Datum age_unwind(PG_FUNCTION_ARGS)
+{
+ agtype *agtype_arg = NULL;
+ ReturnSetInfo *rsi;
+ Tuplestorestate *tuple_store;
+ TupleDesc tupdesc;
+ TupleDesc ret_tdesc;
+ MemoryContext old_cxt, tmp_cxt;
+ bool skipNested = false;
+ agtype_iterator *it;
+ agtype_value v;
+ agtype_iterator_token r;
+
+ /* check for a NULL expr */
+ if (PG_ARGISNULL(0))
+ {
+ PG_RETURN_NULL();
+ }
+
+ agtype_arg = AG_GET_ARG_AGTYPE_P(0);
+ if (!AGT_ROOT_IS_ARRAY(agtype_arg))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot extract elements from an object")));
+ }
+
+ rsi = (ReturnSetInfo *) fcinfo->resultinfo;
+
+ rsi->returnMode = SFRM_Materialize;
+
+ /* it's a simple type, so don't use get_call_result_type() */
+ tupdesc = rsi->expectedDesc;
+
+ old_cxt = MemoryContextSwitchTo(rsi->econtext->ecxt_per_query_memory);
+
+ ret_tdesc = CreateTupleDescCopy(tupdesc);
+ BlessTupleDesc(ret_tdesc);
+ tuple_store =
+ tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
+ false, work_mem);
+
+ MemoryContextSwitchTo(old_cxt);
+
+ tmp_cxt = AllocSetContextCreate(CurrentMemoryContext,
+ "age_unwind temporary cxt",
+ ALLOCSET_DEFAULT_SIZES);
+
+ it = agtype_iterator_init(&agtype_arg->root);
+
+ while ((r = agtype_iterator_next(&it, &v, skipNested)) != WAGT_DONE)
+ {
+ skipNested = true;
+
+ if (r == WAGT_ELEM)
+ {
+ HeapTuple tuple;
+ Datum values[1] = {(Datum) 0};
+ bool nulls[1] = {false};
+
+ /* use the tmp context so we can clean up after each tuple is done
*/
+ old_cxt = MemoryContextSwitchTo(tmp_cxt);
+
+ if (v.type == AGTV_NULL)
+ {
+ /* emit SQL NULL for agtype JSON null (issue #2383) */
+ nulls[0] = true;
+ }
+ else
+ {
+ agtype *val = agtype_value_to_agtype(&v);
+
+ values[0] = PointerGetDatum(val);
+ }
+
+ tuple = heap_form_tuple(ret_tdesc, values, nulls);
+
+ /* switch to the per query context to hold the tuple */
+ MemoryContextSwitchTo(rsi->econtext->ecxt_per_query_memory);
+
+ tuplestore_puttuple(tuple_store, tuple);
+
+ /* now reset the tmp context */
+ MemoryContextSwitchTo(tmp_cxt);
+ MemoryContextReset(tmp_cxt);
+
+ /* reset back to the old context */
+ MemoryContextSwitchTo(old_cxt);
Review Comment:
Inside the per-element loop, `age_unwind` performs extra
`MemoryContextSwitchTo` calls (switching to per-query memory before
`tuplestore_puttuple`, then back to `tmp_cxt`, then back again). `age_unnest`
simply calls `tuplestore_puttuple` directly and resets the temp context, which
is both simpler and avoids additional context switches on large lists. Consider
matching `age_unnest`'s pattern here to reduce overhead and keep the two SRFs
structurally aligned.
```suggestion
tuplestore_puttuple(tuple_store, tuple);
/* reset back to the old context, then clear tmp allocations */
MemoryContextSwitchTo(old_cxt);
MemoryContextReset(tmp_cxt);
```
##########
src/backend/utils/adt/agtype.c:
##########
@@ -12450,6 +12450,115 @@ Datum age_unnest(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
+PG_FUNCTION_INFO_V1(age_unwind);
+/*
+ * Function to convert an agtype array into a set of rows for the Cypher
+ * `UNWIND` clause. Behaves like age_unnest() but, per Neo4j/openCypher
+ * semantics, emits an SQL NULL row for each top-level element that is an
+ * agtype JSON null (AGTV_NULL). This lets null-strict operators such as
+ * count(x) and IS NULL treat `UNWIND [null] AS x` the same as
+ * `WITH null AS x`. Nested nulls inside arrays/objects are preserved as
+ * agtype-null so container semantics are unchanged. See issue #2383.
+ */
+Datum age_unwind(PG_FUNCTION_ARGS)
+{
+ agtype *agtype_arg = NULL;
+ ReturnSetInfo *rsi;
+ Tuplestorestate *tuple_store;
+ TupleDesc tupdesc;
+ TupleDesc ret_tdesc;
+ MemoryContext old_cxt, tmp_cxt;
+ bool skipNested = false;
+ agtype_iterator *it;
+ agtype_value v;
+ agtype_iterator_token r;
+
+ /* check for a NULL expr */
+ if (PG_ARGISNULL(0))
+ {
+ PG_RETURN_NULL();
+ }
+
+ agtype_arg = AG_GET_ARG_AGTYPE_P(0);
+ if (!AGT_ROOT_IS_ARRAY(agtype_arg))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot extract elements from an object")));
Review Comment:
The error raised when the input is not an array uses `errmsg("cannot extract
elements from an object")`, but this branch is also hit for agtype scalars (and
potentially other non-array roots). Consider updating the message to something
type-accurate like "cannot extract elements from a non-array" (or explicitly
handle scalar/object separately) so UNWIND errors are not misleading.
```suggestion
errmsg("cannot extract elements from a non-array")));
```
--
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]
