Conversation
The sanitizer test suite needs the dataset submodule for the demo db; mirror the pattern used in ci-workflow.yml.
Implement docs/multi_parent_lifetime.md's "for now" recommendation: - Keep the owned std::vector<sel_t> parentPositions copy in PackedChildSlices; explicitly document why a pointer/alias into the parent's SelectionVector is unsafe (contents rewritten in place by setToFiltered/setToUnfiltered). - Document the synchronous-consumption lifetime rule: the descriptor is valid only for the current output batch and must not be persisted across a materialization boundary. - Record the deferred multi-parent convention (shared_ptr sel vector + prefix-sum offsets over all parents, zeros allowed) for when the multi-parent packed scan is implemented. - Add a unit test codifying the owned-copy invariant against in-place sel vector mutation.
Implement the deferred design from docs/multi_parent_lifetime.md: Representation (PackedChildSlices): - Replace the owned parentPositions copy with a shared_ptr alias to the bound (parent) chunk state's selection vector plus a prefix-sum offsets vector over ALL served parents (offsets.size() == parentSelSize + 1). Zero-length ranges (parents without children in the batch) are allowed; consumers skip them. The shared_ptr keeps the SelectionVector object alive while still aliasing mutable contents, preserving the synchronous-consumption lifetime rule. Storage (CSRNodeGroup): - Add tryScanCachedTuplesPacked: serves children of as many consecutive parents as fit into one output batch from the row cache, building the combined (filtered) output selection vector, the per-parent prefix sum in RelTableScanState::packedChildOffsets, and the served-parent selection on the bound chunk state (switched to unflat). Handles zero-length lists, capacity-limited partial lists, and cache-window boundaries; parents whose lists span batches are resumed seamlessly. - Single-parent serves (without-cache, in-memory, local storage) keep the existing one-parent-per-batch contract. Operator (ScanRelTable): - Attach the descriptor for both single- and multi-parent batches; the batch shape is derived from the bound vector's selection size. - Drop the speculative append/reserve machinery (superseded by direct offsets construction). Consumer gating: - Multi-parent batches change the factorization contract (bound chunk unflat with the served parents), so they are opt-in per consumer: the plan mapper enables it only for PackedFilteredCount consuming the scan output DIRECTLY (no FactorizedTable/hash-join between, the exact materialization hazard the doc warns about). Plans that fetch nbr node properties through the property-join pipeline keep single-parent batches. PackedFilteredCount: - Attribute counts per parent via the packed slices when present (a batch may span many group keys); keep the legacy single-tuple path when no descriptor is attached. Tests (test/planner/cardinality_test.cpp): - Rework the DataChunkState unit tests for the new representation and document the aliasing semantics in a dedicated test. - Add a storage-level test driving RelTable::scan directly with packing enabled over a 3000-node/6750-edge graph (degree 3, with zero-child parents): asserts multi-parent batches actually occur, the offsets prefix-sum invariants hold, and every edge is served exactly once with correct parent attribution. - Add SQL-level equivalence: the PackedFilteredCount pattern over an nbr property (join pipeline, single-parent batches) produces identical results with the packed extend on and off. Docs: mark the deferred design as implemented in docs/multi_parent_lifetime.md, noting the materialization boundary constraint that keeps the property-join plan shape single-parent.
Debug builds assert in FactorizedTable::copyVectorToUnflatColumn / appendVectorToUnflatTupleBlocks when a flat vector is appended to an unflat column. The implementation already handles this correctly: a flat vector (single selected value) is stored as a one-element unflat overflow value, which is exactly the right representation. This shape is reached by the packed filtered count pipeline: the packed extend keeps its bound node group unflat at plan time (getGroupsPosToFlatten is empty, so no Flatten operators are inserted), while the CSR scan presents the bound chunk flat with selSize 1 per output batch. When the acc-hash-join SIP rewrite places an Accumulate (FactorizedTable writer) directly above the packed extend, the append hits the over-strict assert on DASSERT-enabled (Debug) builds. Release builds run the same path and produce correct results. Relax the asserts to require only that a flat vector carry a single selected value, matching the implemented semantics.
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
Implements the deferred design from
docs/multi_parent_lifetime.md: a multi-parent packed scan for the CSR rel scan path, plus the lifetime/representation documentation work it calls for.Representation (
PackedChildSlices)parentPositions(owned copy of only non-empty parents) →shared_ptr<SelectionVector>aliasing the bound (parent) chunk state's selection vector + prefix-sumoffsetsover all served parents (offsets.size() == parentSelSize + 1).offsets[i] == offsets[i+1]) are allowed and must be skipped by consumers — the "drop parents without matches" invariant moves from scan time to the consumer, exactly as the doc describes.shared_ptrkeeps theSelectionVectorobject alive while still aliasing mutable contents, preserving the synchronous-consumption lifetime rule.Storage (
CSRNodeGroup)CSRNodeGroupScanState::tryScanCachedTuplesPacked(): serves children of as many consecutive parents as fit into one output batch from the CSR row cache. Builds the combined (filtered) output selection vector, the per-parent prefix sum (RelTableScanState::packedChildOffsets), and the served-parent selection on the bound chunk state (switched to unflat).Operator & gating
ScanRelTable::updatePackedChildSlices()attaches the descriptor for both batch shapes (derived from the bound vector's selection size); the speculative append/reserve machinery is removed.PackedFilteredCountconsuming the scan output directly (dynamic_caston the mapped child). Plans that fetch an nbr node property route through a hash join +FactorizedTablematerialization — the exact materialization hazard the doc warns about — and correctly stay single-parent (verified empirically).Consumer
PackedFilteredCountattributes counts per parent via the packed slices when present (a batch may span many group keys); the legacy single-tuple cross-product path is preserved when no descriptor is attached (e.g. when the sink sits above the property-fetch hash join).Tests
RelTable::scandirectly with packing enabled over a 3000-node / 6750-edge graph (degree 3, with zero-child parents): asserts multi-parent batches actually occur, the prefix-sum invariants hold, and every edge is served exactly once with correct parent attribution.PackedFilteredCountpattern over an nbr property (join pipeline, single-parent batches) produces identical results with the packed extend on and off.Docs
docs/multi_parent_lifetime.mdupdated with an implementation-status note, including the materialization-boundary constraint that keeps the property-join plan shape single-parent.Testing
planner_tests(release + ASAN, DASSERTs exercised),rel_tests, and ~50 e2e test groups (match, filter, agg, generic_hash_join, dml_rel, cyclic, path, subquery, transaction, unwind, rel_group, acc, projection, …) all pass.clang-format-18clean.