agtype: propagate null through list slice bounds instead of treating AGTV_NULL as omitted#2400
Conversation
…AGTV_NULL as omitted
agtype_access_slice() decides what to do with each bound in two steps:
1. If the SQL argument is NULL (`PG_ARGISNULL(1|2)`), the caller did
not supply that bound at all (e.g. `list[..2]`) - treat it as
"no bound" and fall back to 0 / array_size. This is correct.
2. Otherwise the argument is non-NULL agtype. If *the agtype value*
inside happens to be AGTV_NULL (e.g. `list[null..2]`), the
existing code also treated it as "no bound" and returned the full
slice. This is wrong. Under Cypher null-propagation semantics
`list[a..b]` is null whenever either bound is null; Neo4j and
Memgraph both return null, and differential testing against both
flagged the divergence (apache#2392).
Change step 2 to PG_RETURN_NULL() when the supplied bound is
AGTV_NULL. Step 1 is untouched, so `[..n]` / `[n..]` still work as
before.
Regression tests are updated accordingly:
* The two pre-existing agtype_access_slice() calls with an
explicit 'null'::agtype argument now expect the null result.
* New bracket-syntax cases cover `[1,2,3][1..null]` and
`[1,2,3][null..2]`, which were the minimal repros on the issue.
Fixes apache#2392
|
@SAY-5 test fails. Actual output did not match the expected output. |
|
@SAY-5 Also, if you have used any AI tool to create this PR, please mention it in the commit message. |
Drop the Cypher-level `[1,2,3][1..null]` / `[null..2]` regression cases. The list-index execution path goes through a separate function from agtype_access_slice and still treats AGTV_NULL as an omitted bound (tracking that fix separately); keeping those cases in the regression file without a matching source change makes the build job fail on diff. Keep the two direct agtype_access_slice tests that actually exercise the fixed code path. Raised by @MuhammadTahaNaveed on apache#2400. Co-developed with an AI coding assistant (Claude Code). Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
|
Thanks @MuhammadTahaNaveed, apologies for the noisy diff. The two Cypher-level cases ( |
2 participants
agtype_access_slice()decides what to do with each bound in two steps:PG_ARGISNULL(1|2)), the caller did not supply that bound at all (e.g.list[..2]) — treat it as "no bound" and fall back to0/array_size. This is correct.AGTV_NULL(e.g.list[null..2]), the existing code also treated it as "no bound" and returned the full slice. This is wrong. Under Cypher null-propagation semanticslist[a..b]is null whenever either bound is null; Neo4j and Memgraph both return null, and differential testing against both flagged the divergence.This PR changes step 2 to
PG_RETURN_NULL()when the supplied bound isAGTV_NULL. Step 1 is untouched, so[..n]/[n..]still work as before.Regression tests are updated accordingly:
agtype_access_slice()calls with an explicit'null'::agtypeargument now expect the null result.[1,2,3][1..null]and[1,2,3][null..2], which were the minimal repros on the issue.