feat(opencypher): enable filter pushdown for list predicates (any/all/none/single) in WHERE clause#3891
Conversation
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 63.87% 63.83% -0.05%
==========================================
Files 1591 1591
Lines 118934 118948 +14
Branches 25275 25277 +2
==========================================
- Hits 75968 75926 -42
- Misses 32489 32548 +59
+ Partials 10477 10474 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the thorough review. I've addressed all points. On the bug reproduction: I reproduced via ArcadeDB Studio using the OpenCypher HTTP API with the exact queries from issue #3888. On current Changes made:
Full Happy to make any further adjustments. |
|
Perfect, thanks!!! |
…h list predicates Locks in the PR #3891 fix by asserting that collectVariables() returns the correct outer variables for any/all/none/single predicates used in a comparison, and excludes the loop-scoped iterator from list comprehensions. All six tests fail on the pre-fix WhereClause implementation.
What does this PR do?
Enables filter pushdown optimization for
any(),all(),none(), andsingle()list predicates when used inWHEREclauses.Previously,
WhereClause.collectExpressionVariables()had no branch forListPredicateExpression, so outer variables referenced inside these predicates (e.g.pinp.name) were never collected. This prevented the query optimizer from recognizing which variables these predicates depend on, causing it to skip them as pushdown candidates entirely.This PR adds the missing branch, correctly collecting outer variables while excluding the loop-scoped iterator variable (e.g.
xinany(x IN list WHERE x = p.name)). The same iterator-exclusion logic is also applied consistently to the existingListComprehensionExpressionbranch, which had the same gap.Files changed:
engine/src/main/java/com/arcadedb/query/opencypher/WhereClause.java: addsListPredicateExpressionbranch and fixesListComprehensionExpressionbranch incollectExpressionVariables()engine/src/test/java/com/arcadedb/query/opencypher/OpenCypherWhereClauseTest.java: addsListPredicateFilterPushdownTestnested class with 5 tests coveringany,NOT any,all,none,singleTesting:
testAnyPredicateInWhere,testNotAnyPredicateInWhere,testAllPredicateInWhere,testNonePredicateInWhere,testSinglePredicateInWhereOpenCyphertest suite passes.