Skip to content

feat: predicate pushdown for WHERE conjunctions on MATCH nodes - #2

Open
villelaitila wants to merge 1 commit into
aneeshdurg:mainfrom
softagram:feat/where-predicate-pushdown
Open

feat: predicate pushdown for WHERE conjunctions on MATCH nodes#2
villelaitila wants to merge 1 commit into
aneeshdurg:mainfrom
softagram:feat/where-predicate-pushdown

Conversation

@villelaitila

Copy link
Copy Markdown
Contributor

Summary

  • When a MATCH pattern is constrained by WHERE var.prop = literal conjunctions, the executor used to enumerate every structural candidate via DFS and filter post-hoc — extremely slow on large graphs with variable-length paths
  • This commit folds recognised equality predicates into the pattern node's property dict before the DFS runs, so DFSMatcher.node_matches prunes wrong candidates immediately
  • Supported literals: string, integer, float, boolean. Null literals fall through to the post-DFS evaluator (Cypher three-valued null semantics)
  • Partial extraction supported: in a mixed AND (a.x = 1 AND a.y = b.y), pushable conjuncts are folded, others stay for post-DFS
  • node_matches now gates on pnode.id_ in node_ids_to_props instead of pnode.properties truthiness, eliminating the need for sentinel objects
  • 27 tests covering happy-path, fallthrough, partial AND, OPTIONAL MATCH, null equality, and edge cases

Test plan

  • 27 unit tests pass (string/int/float/bool literals, reversed operands, OR/NOT/inequality fallthrough, partial AND, OPTIONAL MATCH, null, inline props + WHERE, unbound variables, escaped strings)
  • openCypher TCK suite: 0 unexpected failures
  • Verified identical query results with and without pushdown on all test cases

When a MATCH pattern is constrained by a WHERE clause of the form:

    MATCH (a)-[*1..30]->(b)
    WHERE a.path = '/foo' AND b.path = '/bar'
    RETURN ...

the executor used to enumerate every structural candidate across the
whole graph via DFS and then evaluate the WHERE expression per row in
a pandas DataFrame, discarding almost all of them. On large graphs
with variable-length paths this is extremely slow.

This commit recognises top-level AND conjunctions of the shape
`<variable>.<property> = <literal>` (in either operand order) in the
WHERE AST and folds them into the pattern node's property dict before
the DFS runs. The DFSMatcher already checks per-node property
constraints via node_matches — the pushdown reuses that machinery.

Supported literal types: string, integer, float, and boolean.
Null literals are deliberately not pushed down (Cypher uses
three-valued null-equality semantics that require the post-DFS
evaluator). Unsupported WHERE shapes (OR, NOT, non-equality,
cross-variable comparisons, non-literal RHS) fall through to the
existing post-DFS WHERE evaluation unchanged. Partial extraction
is supported: in a mixed AND, pushable conjuncts are folded while
others remain for the post-DFS filter.

Implementation:
- spycy/predicate_pushdown.py: new module with collect_pushdown_predicates()
  and apply_pushdown() — AST walker + property dict merger.
- spycy/spycy.py: 8-line hook in _process_match after pattern property
  evaluation, before the DFS call.
- spycy/dfsmatcher.py: node_matches gates on `pnode.id_ in
  node_ids_to_props` instead of checking `pnode.properties`, so
  pushdown entries are picked up without mutating the pattern AST.
- test/test_predicate_pushdown.py: 27 tests covering happy-path
  (string/int/float/bool literals, reversed operands, multi-AND),
  fallthrough (OR, NOT, >, <, cross-var, non-literal RHS), partial
  AND (one pushable + one non-pushable conjunct), OPTIONAL MATCH,
  null equality, and edge cases (inline props + WHERE combined,
  unbound variables, escaped strings, missing properties).

No regressions in the openCypher TCK suite.

@aneeshdurg aneeshdurg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for contributing! The actual predicate pushdown implementation and matcher changes seem reasonable.

from spycy.gen.CypherParser import CypherParser


PushdownTriple = Tuple[str, str, Any]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you make this a dataclass instead?

Comment on lines +257 to +265
.replace('\\\\', '\x00')
.replace("\\'", "'")
.replace('\\"', '"')
.replace('\\n', '\n')
.replace('\\r', '\r')
.replace('\\t', '\t')
.replace('\\b', '\b')
.replace('\\f', '\f')
.replace('\x00', '\\'))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure I understand why this is needed?
If it really is needed, shouldn't it also be in the expression_evaluator?

return (var_node.getText(), key.getText())


def _try_extract_literal(non_arith_expr) -> Optional[Any]:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should really be a call to the expression evaluator instead. There's already support for evaluating a literal.

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