Fix UNWIND [null] causing count() to miscount null rows (#2383)

[crprashant]

Fixes #2383.

Problem

Per Neo4j / openCypher semantics, count() must ignore nulls. On Apache AGE:

UNWIND [null] AS x RETURN count(x)   -- returns 1 (buggy), should be 0

The control cases already returned the correct 0:

RETURN count(null)
WITH null AS x RETURN count(x)

Root cause

age_unnest() in src/backend/utils/adt/agtype.c materialized every top-level array element into a heap tuple with nulls[0] = false, even when the element's agtype_value.type == AGTV_NULL. The row that reached count() therefore carried a non-NULL datum wrapping an agtype JSON null, so the aggregate's strict null check never skipped it.

Fix

A naïve fix inside age_unnest() breaks other callers. construct_age_function_name() in src/backend/parser/cypher_expr.c rewrites unnest → age_unnest, so list comprehensions and the predicate functions (all / any / none / single) also route through age_unnest. Those callers deliberately rely on the quirk that AGE's agtype_null > agtype_integer evaluates to agtype_null (truthy), not SQL NULL. Changing the SRF broke five predicate_functions expected results.

Introduce a dedicated SRF age_unwind() with UNWIND-specific null semantics:

• Top-level AGTV_NULL elements → SQL NULL rows so null-strict operators (count, IS NULL, WHERE x IS NOT NULL, …) match WITH null AS x.
• Non-null elements are returned unchanged.
• Nested nulls inside arrays/objects are preserved as agtype-null.

transform_cypher_unwind() is the only call site changed from age_unnest to age_unwind. List comprehensions and predicate functions continue to use age_unnest and their behavior is untouched.

Before / after

-- before
UNWIND [null] AS x RETURN count(x)                  -- 1
UNWIND [1, null, 2, null, 3] AS x RETURN count(x)   -- 5
UNWIND [null] AS x RETURN x IS NULL                 -- false

-- after
UNWIND [null] AS x RETURN count(x)                  -- 0 ✓
UNWIND [1, null, 2, null, 3] AS x RETURN count(x)   -- 3 ✓
UNWIND [null] AS x RETURN x IS NULL                 -- true ✓

Regression tests (regress/sql/cypher_unwind.sql)

• UNWIND [null] RETURN count(x) = 0
• UNWIND [1, null, 2, null, 3] RETURN count(x) = 3
• UNWIND [null] RETURN x IS NULL
• UNWIND [1, null, 2] WITH x WHERE x IS NOT NULL RETURN count(x) = 2
• UNWIND [null] RETURN count(*) = 1 (row still produced)
• UNWIND [[null], {k: null}] RETURN count(x) = 2 (nested nulls preserved)
• UNWIND [null] RETURN x — asserts the row value itself is SQL NULL

Files changed

• src/backend/utils/adt/agtype.c — new age_unwind SRF
• src/backend/parser/cypher_clause.c — UNWIND calls age_unwind
• sql/agtype_typecast.sql — declare age_unwind
• age--1.7.0--y.y.y.sql — add age_unwind for in-place upgrades
• regress/sql/cypher_unwind.sql + regress/expected/cypher_unwind.out

Test results

make installcheck with PostgreSQL 18 on Ubuntu 24.04:

• 32/33 tests pass.
• age_upgrade fails identically on apache/age master HEAD (pre-existing, unrelated; same age_prepare_pg_upgrade already exists error).
• cypher_unwind, predicate_functions, list_comprehension all pass.

[crprashant]

Heads-up for reviewers — just noticed @MuhammadTahaNaveed's #2406 fixes the same root cause (agtype AGTV_NULL not being emitted as SQL NULL from age_unnest) with a broader scope that also covers single() / all() / any() / none() three-valued semantics. That direction was my first instinct too, but I backed off after it altered the predicate_functions regression; #2406 goes further and reworks those predicates to match Neo4j three-valued logic, which is the right call. #2406 subsumes this PR.

Happy to close #2411 in favor of #2406 once it lands. The 7 regression cases in regress/sql/cypher_unwind.sql here specifically cover UNWIND [null] AS x RETURN count(x) = 0 (issue #2383) and a few UNWIND-only boundaries (nested lists, mixed nulls, DISTINCT over UNWIND) that #2406's tests don't exercise — glad to open a small follow-up PR porting just those tests on top of #2406 if that would be useful.

Thanks for the fast work on the null-propagation family of bugs.

[Copilot]

Pull request overview

Fixes Apache AGE’s UNWIND null-row semantics so count(expr) and other null-strict SQL operators correctly ignore top-level JSON null elements produced by UNWIND, aligning behavior with Neo4j/openCypher (issue #2383).

Changes:

• Added a dedicated SRF age_unwind() that emits SQL NULL rows for top-level AGTV_NULL elements while preserving nested agtype nulls.
• Updated Cypher UNWIND transformation to call age_unwind instead of routing through age_unnest.
• Added regression coverage for UNWIND/count/IS NULL behavior and updated expected outputs; added SQL declarations for installation/upgrade.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/backend/utils/adt/agtype.c	Introduces age_unwind() SRF with UNWIND-specific top-level null → SQL NULL behavior.
src/backend/parser/cypher_clause.c	Switches UNWIND clause transformation to invoke age_unwind.
sql/agtype_typecast.sql	Declares ag_catalog.age_unwind(agtype) RETURNS SETOF agtype.
age--1.7.0--y.y.y.sql	Adds age_unwind to the upgrade template for in-place upgrades.
regress/sql/cypher_unwind.sql	Adds regression queries validating UNWIND null semantics and strict-null operator behavior.
regress/expected/cypher_unwind.out	Updates expected results for the new regression coverage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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.

Suggested change

	errmsg("cannot extract elements from an object")));
	errmsg("cannot extract elements from a non-array")));

[Copilot]

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.

Suggested change

	/* 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);
	tuplestore_puttuple(tuple_store, tuple);
	/* reset back to the old context, then clear tmp allocations */
	MemoryContextSwitchTo(old_cxt);
	MemoryContextReset(tmp_cxt);