Repository navigation
fix(scan): do not scan external rel tables before pulling child tuple - #1533
Open
Tyagiquamar wants to merge 2 commits into
Open
Tyagiquamar wants to merge 2 commits into
Tyagiquamar wants to merge 2 commits into
Conversation
Commit c1dcef7 added an unconditional tableInfo.table->initScanState call in ScanRelTable::initLocalStateInternal for non-native RelTables to prevent a crash in ForeignRelTable::scanInternal when sharedState was null. However, for ArrowRelTable and IceDiskRelTable, initScanState reads the bound node ID vector to populate bound node mappings. At initLocalStateInternal time, no child tuple or bound node batch has been fetched yet, leaving the bound vector at default offset 0. When getNextTuplesInternal was called, scan() executed against node offset 0 before pulling the actual child tuple, yielding node 0's relationships in addition to the matching node's relationships. This fix: 1. Removes the premature initScanState call from ScanRelTable::initLocalStateInternal. 2. Guards the scan loops in ScanRelTable::getNextTuplesInternal with scanState->source != TableScanSource::NONE. 3. Updates ForeignRelTable::scanInternal, ArrowRelTable::scanInternal, and IceDiskRelTable::scanInternal to safely return false when source == TableScanSource::NONE or scan state is uninitialized. 4. Marks ForeignRelTableScanState source as COMMITTED during initScanState and resets it to NONE upon scan completion. 5. Adds regression tests covering primary key matches over flat and CSR Arrow relationship tables. Fixes LadybugDB#1527. Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
clang-format-18 in CI expects the ++iceDiskScanState continuation to stay column-aligned with the open paren, as on main. Local clang-format-19 accepts both, so match main exactly. Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
This branch has not been deployed
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 #1527 by preventing
ScanRelTablefrom scanning external relationship tables before the first child tuple (or bound node batch) has been pulled, and ensuring external relationship tables safely returnfalsewhen their scan state is uninitialized or inTableScanSource::NONE.Root Cause & Problem
Commit
c1dcef7301introduced an unconditional call totableInfo.table->initScanState(...)inScanRelTable::initLocalStateInternalfor all non-nativeRelTabletypes:That change was added to prevent
ForeignRelTable::scanInternalfrom throwing or crashing whensharedStatewas null on the very first scan call.However, unlike the assumption that external rel tables do not need bound node information,
ArrowRelTable(both flat and CSR) andIceDiskRelTableuseinitScanStateto readscanState.nodeIDVectorand map bound node offsets to positions:initLocalStateInternaltime, no child tuple has been fetched fromchildren[0]. The bound node vector holds default offset 0.initScanStatemaps offset 0 intoarrowBoundNodeOffsetToSelPos(orboundNodeOffsets) and setssource = TableScanSource::COMMITTED.getNextTuplesInternal, the initial loop executeswhile (tableInfo.table->scan(transaction, *scanState)). BecausesourceisCOMMITTED,scan()scans and emits all relationships for node offset 0 (the first node in the table).children[0]->getNextTuple(...)is finally invoked to pull the actual matching tuple (e.g. from a primary key index lookup), re-initializingscanStatefor the requested node.Technical Solution
initScanStatecall fromScanRelTable::initLocalStateInternal. All rel tables now wait until eitherfetchNextBoundNodeBatch(insourceMode) orchildren[0]->getNextTuple(in non-sourceMode) pulls valid bound nodes before initializing scan state.ScanRelTable::getNextTuplesInternal, guarded both scan loops withscanState->source != TableScanSource::NONEto ensurescan()is never called prior to bound node initialization.ForeignRelTable::scanInternalto safely returnfalsewhensource == TableScanSource::NONEorsharedState == nullptrinstead of throwing aRuntimeException.foreignRelScanState.source = TableScanSource::COMMITTEDinForeignRelTable::initScanState, and reset it toTableScanSource::NONEwhen the foreign scan produces 0 tuples.ArrowRelTable::scanInternalandIceDiskRelTable::scanInternalto checkscanState.source == TableScanSource::NONEand returnfalsesafely when no scan is active.Test Plan
Added automated regression tests in LadybugDB native test suites:
ArrowRelTableTest.MatchPrimaryKeyOverArrowRelTableDoesNotReturnFirstNodeRelsintest/api/arrow_rel_table_test.cpp: verifies that a primary key match query on a flat Arrow rel table returns only the requested node's edges and does not emit node 0's edges, and returns 0 rows when matching a node without relationships.ArrowCsrRelTableTest.MatchPrimaryKeyOverArrowCsrRelTableDoesNotReturnFirstNodeRelsintest/api/arrow_csr_rel_table_test.cpp: verifies identical behavior for CSR layout Arrow rel tables.