fix(opencypher): collect variables from ListPredicateExpression in WHERE clause#3891
fix(opencypher): collect variables from ListPredicateExpression in WHERE clause#3891subha0319 wants to merge 2 commits intoArcadeData:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements variable collection for list predicate expressions in OpenCypher WHERE clauses, ensuring that variables within predicates like 'any()' are correctly identified. Feedback was provided regarding the placement and implementation of the new test cases. Specifically, the tests were added to an inappropriate nested class lacking the necessary vertex type setup, used inconsistent indentation, and utilized JUnit assertions instead of the project's standard AssertJ library. An unnecessary JUnit import should also be removed.
| @Test | ||
| public void testAnyPredicateInWhere() { | ||
| // Cleanup first to ensure a clean state | ||
| database.command("opencypher", "MATCH (n:Person) DETACH DELETE n"); | ||
|
|
||
| database.command("opencypher", | ||
| "CREATE (:Person {name:'Alice'}), (:Person {name:'Bob'}), (:Person {name:'Charlie'})"); | ||
|
|
||
| ResultSet rs = database.query("opencypher", | ||
| "MATCH (p:Person) WHERE any(x IN ['Alice'] WHERE x = p.name) RETURN p.name AS name ORDER BY name"); | ||
|
|
||
| List<String> names = new ArrayList<>(); | ||
| while (rs.hasNext()) names.add(rs.next().getProperty("name")); | ||
|
|
||
| assertEquals(List.of("Alice"), names); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNotAnyPredicateInWhere() { | ||
| // Cleanup first to ensure a clean state | ||
| database.command("opencypher", "MATCH (n:Person) DETACH DELETE n"); | ||
|
|
||
| database.command("opencypher", | ||
| "CREATE (:Person {name:'Alice'}), (:Person {name:'Bob'}), (:Person {name:'Charlie'})"); | ||
|
|
||
| ResultSet rs = database.query("opencypher", | ||
| "MATCH (p:Person) WHERE NOT any(x IN ['Alice'] WHERE x = p.name) RETURN p.name AS name ORDER BY name"); | ||
|
|
||
| List<String> names = new ArrayList<>(); | ||
| while (rs.hasNext()) names.add(rs.next().getProperty("name")); | ||
|
|
||
| assertEquals(List.of("Bob", "Charlie"), names); | ||
| } | ||
| } |
There was a problem hiding this comment.
These tests are logically misplaced inside the NotOperatorParenthesesRegression nested class, which is intended for a different set of regression tests. Furthermore, the setUp method of this nested class does not create the Person vertex type, which will cause these tests to fail. Moving them to the main OpenCypherWhereClauseTest class ensures they use the correct database setup and follow the file's 2-space indentation and AssertJ usage style.
}
@Test
void testAnyPredicateInWhere() {
database.command("opencypher", "MATCH (n:Person) DETACH DELETE n");
database.command("opencypher",
"CREATE (:Person {name:'Alice'}), (:Person {name:'Bob'}), (:Person {name:'Charlie'})");
ResultSet rs = database.query("opencypher",
"MATCH (p:Person) WHERE any(x IN ['Alice'] WHERE x = p.name) RETURN p.name AS name ORDER BY name");
List<String> names = new ArrayList<>();
while (rs.hasNext()) names.add(rs.next().getProperty("name"));
assertThat(names).containsExactly("Alice");
}
@Test
void testNotAnyPredicateInWhere() {
database.command("opencypher", "MATCH (n:Person) DETACH DELETE n");
database.command("opencypher",
"CREATE (:Person {name:'Alice'}), (:Person {name:'Bob'}), (:Person {name:'Charlie'})");
ResultSet rs = database.query("opencypher",
"MATCH (p:Person) WHERE NOT any(x IN ['Alice'] WHERE x = p.name) RETURN p.name AS name ORDER BY name");
List<String> names = new ArrayList<>();
while (rs.hasNext()) names.add(rs.next().getProperty("name"));
assertThat(names).containsExactly("Bob", "Charlie");
}|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; |
There was a problem hiding this comment.
This import is unnecessary if we stick to AssertJ (assertThat) which is used throughout the rest of the file. It's better to maintain consistency in the testing framework used.
Up to standards ✅🟢 Issues
|
|
Thanks for the PR! Here are my findings after reviewing the change and reproducing locally. Critical1. The new tests pass without the fix. 2. Tests are placed in the wrong nested class. 3. Project conventions not followed. Style
Design / completeness
What's good
Requested follow-upBecause the tests don't distinguish "with fix" from "without fix" on
Once the points above are addressed, I'm happy to take another look. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3891 +/- ##
==========================================
- Coverage 64.71% 57.50% -7.22%
==========================================
Files 1581 1581
Lines 117023 117030 +7
Branches 24858 24860 +2
==========================================
- Hits 75735 67299 -8436
- Misses 30925 40148 +9223
+ Partials 10363 9583 -780 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
What does this PR do?
Fixes a bug where
any(),none(),all(), andsingle()list predicates were silently ignored or inverted when used as filter conditions in aWHEREclause. The predicates worked correctly when used as expressions inRETURN, but had no effect (or the opposite effect) inWHERE.The fix adds handling for
ListPredicateExpressioninWhereClause.collectExpressionVariables()so that outer variables referenced inside the predicate (e.g.pfromp.name) are correctly collected, while the loop-scoped iterator variable (e.g.x) is excluded as it is locally bound.Motivation
Issue #3888 was reported with a clear reproduction case: a query like:
was returning all rows (
Alice,Bob,Charlie) instead of onlyAlice. The complementaryNOT any(...)form also behaved incorrectly, returning no rows when all non-matching rows should survive. Neo4j handles both cases correctly.The root cause was that
WhereClause.collectExpressionVariables()had no branch forListPredicateExpression. This caused variable extraction to return an empty set, which made theextractForVariables()logic skip the predicate entirely during WHERE filter evaluation hence, treating it as always true.Related issues
any(...)list predicates may be evaluated correctly as expressions inRETURN, but ignored or inverted when used inWHERE. #3888 —any(...)list predicates may be evaluated correctly as expressions in RETURN, but ignored or inverted when used in WHEREAdditional Notes
WhereClause.javaand no changes toListPredicateExpression.javaor the executor layer were needed, as the per-item evaluation logic (evaluateAny,testItem, etc.) was already correct.mvn clean packageon Windows are pre-existing and reproducible on the upstreamArcadeData/arcadedbmain branch without any of this PR's changes. These failures are unrelated to the list predicate fix.testAnyPredicateInWhereandtestNotAnyPredicateInWherepass successfully.any(),none(),all(),single().Checklist
mvn clean packagecommand