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.

[gregfelice]

Reviewed the new age_unwind SRF. The implementation is correct and cleanly scoped.

Implementation. age_unwind mirrors age_unnest's structure (SFRM_Materialize, tuplestore in per-query memory, per-element tmp_cxt reset) and diverges only to set nulls[0]=true for a top-level AGTV_NULL. Because the iterator runs with skipNested=true, only top-level elements are emitted, so nested nulls inside arrays/objects are correctly preserved as agtype-null. Both Copilot notes (the misleading non-array error message and the redundant per-element context switching) are already addressed in 50b4253, which the current head reflects. No correctness concerns.

Relationship to #2406 — a scoping call for the maintainers. This PR and #2406 fix the same underlying UNWIND [null] issue two different ways:

• This PR is the surgical option: a separate SRF, with only transform_cypher_unwind repointed to it, leaving age_unnest untouched. Lowest risk — it can't regress list comprehensions or the predicate functions. The trade-off is that it intentionally leaves the related list-comprehension null-filter bug (#2393) and the predicate-function three-valued logic unfixed, since those callers currently lean on AGE's agtype_null > agtype_integer being truthy.
• #2406 takes the root-cause route: it fixes age_unnest itself and rewrites the predicate-function 3VL so that truthy-null quirk becomes proper three-valued logic, fixing UNWIND, list comprehensions, and single() together.

Per openCypher, null > 0 should be null rather than truthy, so #2406 is the more correct end state; this PR is the smaller, safer incremental step. Worth a maintainer decision on which scope to take, since whichever lands first will require the other to be reworked.

Before merge (independent of the above): the PR is currently in conflict with master (needs a rebase), and I don't see a CI run on it — the gate wants a green PG18 cassert build, so it'd be good to get CI to run once rebased. Locally-reported 32/33 with the known-pre-existing age_upgrade failure matches what master does today.