Repository navigation
Group AND/OR under NOT in Neo4jVectorFilterExpressionConverter - #7222
Open
Rishavbhattarai wants to merge 1 commit into
Open
Rishavbhattarai wants to merge 1 commit into
Rishavbhattarai wants to merge 1 commit into
Conversation
Neo4jVectorFilterExpressionConverter.doNot() wrote the negated expression without parentheses. For NOT(a AND b) it emitted `NOT a AND b`, which Cypher reads as `(NOT a) AND b`, so the filter returned the wrong documents. The same happened for OR. Wrap an AND or OR operand in a group before prefixing it with NOT. Other operands, such as the IN used for NIN, are unchanged. Add converter tests for NOT over AND and OR, and an integration test that runs both filters against Neo4j. Signed-off-by: Rishavbhattarai <bhattarai.rishav@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Neo4jVectorFilterExpressionConverter.doNot()prefixes the negated expression withNOTbut adds no parentheses. When the operand is an AND or OR that isn't wrapped in aGroup, the Cypher means something else:b.not(b.and(b.eq("genre", "drama"), b.gt("year", 2020)))NOT node.genre = "drama" AND node.year > 2020(NOT genre = "drama") AND year > 2020b.not(b.or(b.eq("genre", "drama"), b.gt("year", 2020)))NOT node.genre = "drama" OR node.year > 2020(NOT genre = "drama") OR year > 2020(
metadata.prefixes shortened.) Against Neo4j 5.24 with four documents, the OR filter returned 3 documents whereSimpleVectorStorereturns 1, and the AND filter returned 1 where it returns 3. The other converters avoid this because the defaultdoNot()pushes the negation down into the terms. Neo4j overrides it.The fix wraps an AND or OR operand in a
Groupbefore addingNOT, so it emitsNOT (a AND b). Other operands are unchanged, so NIN still producesNOT node.x IN [...].Tests:
Neo4jVectorFilterExpressionConverterTestsfor NOT over an ungrouped AND and OR.searchWithNegatedCompoundFilterinNeo4jVectorStoreITruns both filters throughFilterExpressionBuilder. That class needsOPENAI_API_KEY, so I ran it locally with aFixedDimensionEmbeddingModelswapped in (not part of this PR): it passes with the change and fails without it, andsearchWithFiltersstill passes../mvnw -pl vector-stores/spring-ai-neo4j-store testpasses (31 tests).Related: #6818 changes the parser so that
NOTbinds tighter thanAND/ORin text filters. This PR is about the expression tree and fixes the builder case either way.