Skip to content

feat: support single-argument properties() for node, rel, and struct - #832

Open
Mrhs121 wants to merge 3 commits into
LadybugDB:mainfrom
Mrhs121:new-feature
Open

feat: support single-argument properties() for node, rel, and struct#832
Mrhs121 wants to merge 3 commits into
LadybugDB:mainfrom
Mrhs121:new-feature

Conversation

@Mrhs121

@Mrhs121 Mrhs121 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

related to #831

@adsharma
adsharma self-requested a review August 22, 2026 17:32
@adsharma

Copy link
Copy Markdown
Contributor

Nice feature overall. Two correctness concerns and a nit:

  1. Null propagation: neither singleArgCompileFunc nor singleArgExecFunc writes the result's outer null mask. For an unflat input (the common case) exec hits the sel-vector equality check and skips entirely, so OPTIONAL MATCH (p:person {ID: 999}) RETURN properties(p) yields a non-null struct — and on the reference path its fields point at the null node's uninitialized child vectors (possibly stale data). openCypher says properties(null) is null. Please propagate outer nulls (or always do a sel-aligned copy) and add an OPTIONAL MATCH regression test.

  2. Fragile invariant: correctness relies on resolveResultStateFromChildren assigning the child's state before compileFunc runs, so the state == check in compile matches the sel-pointer check in exec; if they ever disagree, the result is silently garbage rather than an error. Please document the contract, or unify both checks on parameters[0]->state.get() == result.state.get().

Nit: if (parameterSelVector == resultSelVector) continue; in singleArgExecFunc is loop-invariant — hoist it above the loop.

@adsharma adsharma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Requesting changes for the OPTIONAL MATCH and null handling. The other two comments are minor.

@Mrhs121

Mrhs121 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for your review! I have addressed all the comments in the latest commit:

  1. Null propagation: Added outer null mask propagation in singleArgExecFunc using setNullFromBits for unflat vectors and setNull for flat/filtered vectors, ensuring properties(null) evaluates to null. Added regression tests for OPTIONAL MATCH (null node / null relationship) and properties(null).
  2. Contract & state check: Documented the zero-copy referencing contract in singleArgCompileFunc and unified the state check on parameters[0]->state.get() == result->state.get().
  3. Loop-invariant hoisted: Hoisted the parameterSelVector == resultSelVector check out of the loop as an early return for the reference path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants