Fix Gremlin fractional index bound, MATCH WHERE evaluated twice, anti-join push-down and filter placement (#9206 #9203 #9202 #9167) - #9271

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Merge Risk: 🔵 Low · up to 0773f

Relationship pattern predicates may report a connection through a dangling edge pointer where they previously skipped it. This is a narrow edge case, so the change is mergeable with owner awareness.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 0773f

The changes are bounded to query execution, but avoiding relationship-record reads may also avoid their access checks. This could expose protected connectivity between otherwise readable vertices. Endpoint constraints remain enforced; an authorization bypass has not been verified.

Retained concerns

• Medium · security · inferred: Segment-only relationship existence checks may weaken edge visibility enforcement. A query user who can bind both vertices but cannot read their connecting edge may obtain a connectivity answer without the edge-record read that previously exercised record permissions. Equivalent authorization on adjacency probes remains unproven; this is an inferred exposure, not a verified bypass.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review

I read the diff only and did not build or run the tests. Overall this is a careful change: each fix is scoped, has a regression test, and the Javadoc explains the reasoning. A few points follow.

Correctness and risk

1. openCypher: a MATCH ... WHERE planned by the cost-based optimizer is evaluated twice per row (FilterOperator in the physical plan, then FilterPropertiesStep), so WHERE rand() < 0.5 keeps a quarter of the rows #9202, dropping FilterPropertiesStep for MATCH WHERE. This is correct only if every optimizer-planned MATCH-level WHERE is applied by the physical plan. The PR text says applyFilterPushdown or the disconnected-pattern join planner covers this. The tests only cover a leading MATCH, and a second MATCH after a MATCH ... WHERE. I'd add cases where the old step might have been the only filter:

   • a MATCH after WITH/UNWIND whose WHERE references an outer variable
   • a MATCH with several comma-separated patterns and a WHERE spanning them
   • a WHERE containing a pattern predicate or EXISTS {}
   • a WHERE that reads a variable the optimizer does not treat as a pattern variable (for example a WITH alias)

   patternVariablesRead already returns an empty set for that last case, which keeps it on top, so that path seems safe. It would still be good to see it asserted.

2. placeConjunctsOnBindingHop.

   • A conjunct that reads only an outer variable (a WITH alias) gets reads.isEmpty() and stays at the top. That is the safe choice.
   • rand() or other non-deterministic conjuncts are now evaluated at a different row multiplicity than before. That is intended, and the semantics are unspecified anyway.
   • Moving a conjunct below later hops could change behavior if a later hop is an optional expand. The chain check allows only ExpandAll, ExpandInto and FilterOperator. Please confirm that ExpandAll is never used for an OPTIONAL MATCH hop, since filtering below an optional hop changes null-row semantics. A test with OPTIONAL MATCH plus a WHERE would pin this down.
   • boundAt.add(target, boundAt.get(target)) shares the same Set instance between two list slots. It is never mutated afterwards, so it is fine, but a short comment would help.

3. nodeInequalityOperands for a.p <> b.p. The uniqueness proof (same single label, mandatory, not null, single-property UNIQUE index) looks sound. Two small points:

   • getPolymorphicIndexByProperties may return a non-unique index while a unique one also exists on the same property. Check that it prefers the unique one, or scan all indexes on the property.
   • id(x) <> id(y) accepts any VariableExpression argument. If x could be a relationship variable in the chain-count path, the push-down should decline. The existing code presumably checks node variables later, but the new id() form deserves an explicit test.

4. PatternPredicateExpression.checkRelationshipExists, segment-only path.

   • For the outgoing case, when isConnectedTo returns false the code falls through to the later logic. For undirected patterns it then checks IN. That seems right, but the isOutgoing && segmentOnly / else if (isOutgoing) structure is hard to follow. Restructuring it so the segment path is a single early branch would be clearer.
   • existsType is checked for every type on every row. Hoisting that check, or catching the type once per predicate, would avoid a schema lookup per row on hot paths.

5. Gremlin, ArcadeFilterByIndexStep.openCursor. The cast (Number) bound[0] relies on isInexact returning true only for Number instances. Please confirm that, or add an instanceof guard. Returning null for "no key can match" and mapping it to EmptyIterator is clean.

Style (CLAUDE.md)

• Fully qualified names are avoided. Good.
• Tests use the assertThat(...) style. Good.
• The tests are not tagged, which is fine given their size. The rand-based statistical tests use fixed seeds for data but not for rand(). The bounds (0.45-0.55 over 50k samples, 0.4-0.6 over about 10k) are very wide relative to the standard error, so flakiness risk is negligible.
• Test DB paths are fixed under ./target/databases/... and use the drop-if-exists pattern. That matches existing tests.

Tests worth adding

• OPTIONAL MATCH with a WHERE, to guard the filter placement.
• A GAV chain and a join case, to prove they keep the old placement (the PR says they do, but no test asserts it).
• A negative test for id(rel) <> id(node) in the count push-down.
• A Gremlin test with a non-integral index key type (for example DOUBLE) and with a String bound, to prove the new branch leaves them untouched.

No security concerns, and I see no new dependencies. Nice work.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of PR 9271. I read the diff only and did not build or run the tests. No blocking bugs found; the change is well scoped and the tests cover the main behaviours. Items I would want covered by extra tests before merging: (1) Removing the post-optimizer FilterPropertiesStep (#9202) relies on every optimizer-planned MATCH WHERE being applied in the physical plan. Please add a second MATCH whose WHERE references only earlier-bound variables, and a variable-length hop with a WHERE, since a dropped predicate silently returns wrong results. (3) propertyIdentifiesNode: check labels with subtypes (unique index on a parent type or only on a subtype) and confirm two distinct nodes can never share a value there; otherwise decline the push-down. (4) PatternPredicateExpression.isConnectedTo: confirm Vertex.isConnectedTo(other, direction, type) handles edge subtypes like getEdges(direction, types) does; the segment test does not cover inheritance or self-loops. Smaller notes: (2) placeConjunctsOnBindingHop changes how often non-deterministic predicates such as rand() run, which is intended but visible; the shared boundAt set is only safe because sets are never mutated afterwards, so a guard comment would help. (5) In the Gremlin openCursor, the (Number) cast is only reached when isInexact is true; a short comment would help, and please check no other caller is affected by the new null return. The rand() statistical tests use wide bounds over about 10k samples, so flake risk is very low. The negative tests for non-unique and nullable properties and for id(r) are good.

Review

Overall this is a well-scoped change. Each fix has a regression test, and the comments say why the code is the way it is. Notes below, none blocking.

#9206 (fractional bound)

• I did not read the ArcadeFilterByIndexStep hunk in detail. Please confirm Issue9206FractionalBoundIndexTest covers NaN, +/-Infinity, bounds beyond the long range (ceil/floor overflow), and the empty-cursor case for both gt and lt.

#9202 (double filter)

• Removing the second FilterPropertiesStep is right, but it now relies on every optimizer path applying the MATCH WHERE, including fallback and disconnected-pattern paths. The tests cover the simple chain and a second MATCH. A test with OPTIONAL MATCH ... WHERE and a disconnected pattern would lock this in.
• The rand() statistical tests are fine as written. 50k samples keeps the 0.45-0.55 band safe.

#9203 (anti-join, segment answer, conjunct placement)

• propertyIdentifiesNode is conservative: it needs a single label, a mandatory and not-null property, and a single-property UNIQUE index. That is good. Check two edge cases:
  • A type whose unique index is NOTUNIQUE, or is a full-text or partial index. index.isUnique() should cover this, but it is worth a test.
  • The index being dropped between planning and execution. If plans are cached, the push-down could outlive its precondition. Check whether the plan cache is invalidated on schema change.
• placeConjunctsOnBindingHop moves non-deterministic predicates (rand(), anything that reads timestamp()) to a different place in the plan. This is probably acceptable since the evaluation count was already unspecified. A guard that leaves non-deterministic conjuncts at the top would make the semantics more predictable.
• It also mutates the operator chain in place (above.setChild(filter)). A hop that is optional or null-extending (OPTIONAL MATCH expands) must not be in the chain. The instanceof ExpandAll/ExpandInto/FilterOperator check should guarantee that, but please confirm ExpandAll has no optional variant, because a filter below an optional hop changes results.
• The boundAt.add(0, ...) loop is O(n^2) in chain length. That is negligible for chains, but a plain array filled in reverse would be cleaner and matches the project's performance preference.
• PatternPredicateExpression.segmentOnly with Vertex.isConnectedTo is a nice win. Please confirm undirected BOTH with multiple types behaves the same as the old path for self-loops and for parallel edges. The tests cover directions and an unknown type, but not self-loops.
• extractSimpleInequality and the anti-join extraction now return a 3-element String[]. A tiny record would be safer than positional ineq[2] indexes, but this is style only.

Style (CLAUDE.md)

• The tests use assertThat(...), which is good. Check that the Issue92xx tests clean up under ./target/databases, which they do.
• There is a stray double comment in propertyIdentifiesNode (two consecutive comment blocks before the loop). Consider merging them.

Security and performance

• No security concerns. The push-down is gated on schema guarantees, and skipping edge-record loads is a clear performance win.

Nice work.

Review

I read the diff only. I did not build the PR or run the tests. Overall the changes are well reasoned, the comments explain the why, and each issue has a regression test. A few points are worth checking before merge.

Correctness and risk

1. openCypher: a MATCH ... WHERE planned by the cost-based optimizer is evaluated twice per row (FilterOperator in the physical plan, then FilterPropertiesStep), so WHERE rand() < 0.5 keeps a quarter of the rows #9202, removing the FilterPropertiesStep for MATCH WHERE. The new comment says every MATCH-level WHERE is applied by the physical plan, either through applyFilterPushdown or the disconnected-pattern join planner. That is a global invariant, and the tests only cover two shapes (single MATCH, and a second MATCH after the first). Please add tests that confirm a WHERE is still applied, with correct results, for:

   • OPTIONAL MATCH ... WHERE
   • a MATCH after WITH/UNWIND
   • a MATCH with several disconnected patterns plus WHERE
   • a MATCH where the optimizer falls back to a path that never adds a Filter (for example a WHERE referencing a path variable or an outer-scope variable)

   If any such path doesn't apply the WHERE, this change silently drops the filter. That is worse than evaluating it twice.

2. openCypher: a negated pattern predicate between two bound nodes costs one full edge-list walk with record loads per row, placed above the last hop; LSQB Q9 written with a property inequality is 80x to 110x slower than with a node inequality and 150x to 180x slower than a loop over the API #9203, placeConjunctsOnBindingHop.

   • Please confirm that ExpandAll/ExpandInto are never used for OPTIONAL MATCH. A filter moved below an optional expand changes the semantics: the row would be null-extended instead of dropped.
   • Moving a conjunct below a hop changes how many times it is evaluated. Conjuncts that read no pattern variable are kept on top, so rand() is safe. A conjunct that mixes a pattern variable with a non-deterministic function, such as a.x > rand(), would now be evaluated per path instead of per row. That is probably acceptable, but it is worth a test or a note.
   • boundAt.add(target, boundAt.get(target)) shares the set between operators. The comment says the sets are never mutated afterwards, which holds today but is fragile. An unmodifiable view would enforce it cheaply.

3. propertyIdentifiesNode.

   • The logic is sound: same single label, mandatory, not null, and a single-property UNIQUE index. Please confirm that getAllIndexes(true) cannot return a partial or filtered unique index, if the engine has those.
   • Consider a test with a subtype that carries its own unique index, to pin the "walks UP" claim in the comment.

4. PatternPredicateExpression segment-only path.

   • schema.existsType(type) is called per type per row. It is cheap, but you could resolve it once per predicate instead.
   • For undirected patterns, please make sure a self-loop is still handled the same as the old edge-iteration path. A small test for (a)-[:K]-(a) would cover it.

5. Gremlin: has(x, lt(29.5)) answered through an index on an INTEGER, LONG, or SHORT property misses the 29 (the #9021 truncated bound, in ArcadeFilterByIndexStep) #9206 Gremlin.

   • Returning null from openCursor() is a little awkward. An explicit empty-cursor constant would be clearer, though the null check is fine as is.
   • Please verify that IntegralKeyBound.isInexact handles NaN and +/-Infinity, and doubles beyond the long range. The test file should include them if it doesn't already.
   • The inclusive = true widening relies on the downstream HasStep re-check. Please make sure that step is always present when ArcadeFilterByIndexStep is inserted, so no false positives leak.

Style (CLAUDE.md)

• The code uses final and pattern-matching instanceof consistently, and has no FQNs or leftover System.out.
• The new tests use assertj properly.
• The probabilistic rand() tests are statistically sound, with about 50k samples against a 0.45-0.55 band. Please confirm the runtime is small, or tag the class @Tag("slow") if not.

Performance

The changes are positive: fewer double evaluations, no edge record loads for plain pattern predicates, and earlier filtering. placeConjunctsOnBindingHop runs at plan time only, and its allocation cost is negligible.

Security

I found nothing of concern.

Nice work. I would mainly like the extra coverage for item 1 and the OPTIONAL MATCH question in item 2 before merging.

Codecov Report

❌ Patch coverage is 77.85235% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.90%. Comparing base (bcae045) to head (0773fac).
⚠️ Report is 27 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9271      +/-   ##
============================================
- Coverage     76.90%   76.90%   -0.01%     
+ Complexity     3983     3981       -2     
============================================
  Files          2127     2128       +1     
  Lines        213261   213552     +291     
  Branches      45311    45397      +86     
============================================
+ Hits         164015   164235     +220     
- Misses        31736    31770      +34     
- Partials      17510    17547      +37     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.