Fix silent row loss when LOAD FROM/UNWIND feeds a MATCH primary key predicate - #864
Merged
Conversation
…redicate Fixes #861 The projection push-down optimizer did not visit the key expression of LogicalQueryPrimaryKeyLookup, so a scan column referenced only as the lookup key was marked as unused and pruned from the scan's columnSkips (e.g. LOAD FROM csv/table-function output). The scan then left the key vector unwritten, the key evaluated to NULL for every row, and the lookup silently returned zero rows - dropping any MATCH/SET/MERGE hanging off it without error. Query shapes affected (all silently matched 0 rows): LOAD FROM 'updates.csv' (header=true) MATCH (p:Products {handle: handle}) SET p.description = description UNWIND [...] AS x MATCH (p:Products {handle: x}) ... The optimizer now collects the lookup's key (and node ID) expressions as in-use, keeping the referenced scan column alive. Explicitly visit QueryPrimaryKeyLookup in ProjectionPushDownOptimizer. Adds regression cases to load_from.test covering the property-map predicate, the equivalent WHERE form, and a SET through the lookup.
While auditing ProjectionPushDownOperator coverage after the primary key lookup fix, two more operators were found that consume expressions which are not part of their own output schema but were silently skipped by the visitor (they were not even present in LogicalOperatorVisitor's dispatch switch): - INDEX_LOOK_UP (LogicalPrimaryKeyLookup): evaluates key expressions on top of the copy-from source scan. Currently safe only because visitCopyFrom happens to collect all source columns and COPY_FROM always sits above it; any change to key binding would reintroduce the same silent-NULL failure mode as #861. - UNWIND_DEDUPLICATE: consumes keyExpressions that are not part of its output schema; currently kept alive only because they overlap with expressions collected through the MERGE above it. Add dispatch cases for both operators to LogicalOperatorVisitor and override them in ProjectionPushDownOptimizer to collect the lookup keys (and warning expressions) and the dedup keys as in-use.
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.
Summary
Fixes #861
LOAD FROM <source>followed directly byMATCH (n:Label {pk: col})— wherecolis a scan column — matched zero rows silently on 0.19.0–0.20.0. No error was raised, the query reported success, and anySET/MERGEhanging off theMATCHnever executed:Root cause
The planner legitimately rewrites a single-node
MATCHwith a primary-key equality predicate into aQUERY_PRIMARY_KEY_LOOKUPwhose key is evaluated on top of the outer (scan) pipeline. However,ProjectionPushDownOptimizernever visitedLogicalQueryPrimaryKeyLookup, so its key expression was never collected as in-use.visitTableFunctionCallthen marked the referenced scan column as skippable (columnSkips) and the physical scan stopped writing that vector. The key evaluator subsequently read an unwritten vector, the key evaluated to NULL for every row,lookupPKfailed, and the lookup returned an empty result — silently.This also explains the report's observations:
LOAD FROM df MATCH (p:Products {handle: handle}) SET ...failed silently (the key column was pruned from the scan).WITH handle AS hbetween the scan and theMATCHworked: the intermediatePROJECTIONstarts a fresh push-down pass that keeps the column alive.{description: description}) worked: it becomes a regular filter, which collects its predicate.MATCH (p:Products {handle: 'a'})(literal key) worked: no outer dependency, no pruning.Fix
Override
visitQueryPrimaryKeyLookupinProjectionPushDownOptimizerto collect the lookup's key and node-ID expressions as in-use, so the scan column they reference is kept alive. This works for both the raw-variable case (the issue's{pk: col}) and arbitrary key expressions (e.g. the implicit CAST inserted for aSERIALprimary key), sincecollectExpressionsInUserecurses into children.Testing
test/test_files/load_from/load_from.test:LOAD FROM ... MATCH (p:person {ID: id}) RETURN COUNT(*)→ 5 (property-map form)LOAD FROM ... MATCH (p:person) WHERE p.ID = id RETURN COUNT(*)→ 5 (equivalentWHEREform)LOAD FROM ... MATCH (p:person {ID: id}) SET p.fName = fName RETURN p.fName ORDER BY id→ 5 rows (verifiesSETexecutes through the lookup)e2e_testfull run: 1960 passed; the only 2 failures (copy_to_csv.CopyToInvalidCase,dictionary_bug~...AnonymousParquetDeleteReload) are pre-existing environment issues (missingdataset/emptyfixtures; JSON export extension not built) and fail identically without this change.optimizer_test,planner_tests,binder_test,api_testall pass.