perf(r2rml): memoize dim parent lookups across child batches (PR-4) - #1485
Closed
aaj3f wants to merge 6 commits into
Closed
perf(r2rml): memoize dim parent lookups across child batches (PR-4)#1485aaj3f wants to merge 6 commits into
aaj3f wants to merge 6 commits into
Conversation
… scan operator (PR-4) The correlated R2RML scan rebuilt every RefObjectMap parent lookup HashMap per 1000-row child batch (build_progress), so a fact⋈dim query re-scanned its dimension parents ~once per batch: q008 (fact⋈dim GROUP BY revenue rollup) burned 123+ table scans and DNF'd. Cache ParentLookups across batches on the operator (Arc-shared, keyed by LookupCacheKey), window- capped: a lookup larger than one materialize window is used for its batch but NOT retained — a fact-as-parent (q015) falls through to today's per-batch build, so the cache cannot OOM. Kill switch FLUREE_R2RML_PARENT_MEMO (default on), read per-operator at construction so both regimes are deterministically testable. Live SF01: q008 memo ON = ok, 8 scans (DNF -> completing); OFF = DNF@75s, 123+ scans — the on/off scan split is load-immune and self-attributing. q008's hash mismatch is pre-existing (identical both switch states; F9-class). q015 fact-parent: no OOM (RSS ~538MB), falls through as designed. Native corpus 55/55, 0 hash mismatches. CI-enforced counting test: 5 child batches against a mock provider = exactly 1 parent scan memo-on, 5 memo-off. Query suite 1201/1201. EXPLICITLY OUT OF SCOPE: q050's correlated-OPTIONAL per-row operator rebuild, which resets any operator-scoped cache — PR-4b will widen inner_pattern_is_hash_join_safe (optional.rs:1418) so R2RML OPTIONAL inners take the batched hash-left-join instead of per-row rebuild.
Contributor
Author
|
Correction that STRENGTHENS the evidence: the PR body's "q008's hash mismatch is pre-existing (F9-class)" is wrong — there is no q008 hash divergence at all. The reported mismatch was an artifact of an ad-hoc diagnostic script ( |
…ched OPTIONAL hash-join Why widening inner_pattern_is_hash_join_safe for Pattern::R2rml is likely sound (same seeded construction as the per-row path; referenced_vars has an R2rml arm), the shape partition (scalar POM + single-valued ref = safe Triple-analogs = exactly q050's inner; star/type-var sound-but-test-first; object-only-correlation sound-unoptimized), and the one soundness-critical prerequisite: R2rmlPattern::referenced_vars completeness (P1). Plus the q008 false-mismatch harness note (JSONL meta-line jq artifact).
…; q008 confirmed deterministically correct
…t-memo # Conflicts: # docs/audit/2026-07-virtual-dataset-perf/04-findings-register.md
The restack merge brings the ancestor per-PR rustfmt commits (f8f5206/ a254f42/12102a075), clearing the inherited stack debt; this formats the two PR-4-authored hunks (memo build_parent_lookup call, memo test literals) so cargo fmt --all --check is green at this HEAD — the review's blocking gate item.
…, child-scan pin, family switch All four #1485 review items (the one blocking + all three optional), plus restack fallout: - Key-name doc accuracy (requested): the memo key is (parent TriplesMap IRI, join_cols) — rom.parent_triples_map — NOT (parent_table, ...). Two parent TMs over one table can render different subject IRIs from the same join key (different subject templates), so a table-name key would replay the wrong lookup; both the operator-field and TmStream docs now say so explicitly to head off a colliding-key "optimization". - Relabel consistency (self-caught while fixing the above): the field doc and the memo-hit comment still attributed the fix to q050 — stale pre-relabel drafts. q050 is NOT fixed by this memo (its correlated OPTIONAL rebuilds the operator per row, resetting operator-scoped caches; PR-4b). Attribution corrected to q008 (123+ parent scans DNF -> 8). - Retention/window coupling (optional): one note at the cap — materialize_window_rows() is env-tunable (FLUREE_R2RML_MATERIALIZE_WINDOW_ROWS), so raising the window also raises per-key memo retention; intentional (both bound the same working-set notion), but a window bump knowingly buys a bigger cache. - Child-scan differential (optional): the scan-counting test now also pins the child ("orders") count — 1 in BOTH regimes, because the pre-existing scan_cache already dedupes the child scan across batches (the naive per-batch expectation of 5 was wrong; the assertion found that immediately). The memo is thereby proven to change parent behavior only. - Family switch consistency (restack follow-on): parent_memo_enabled() now routes through r2rml::env_switch_enabled (the PR-3 tip helper), so "no" disables it like every other R2RML switch. - Restack fallout: two PR-3-side materialize_batch tests Arc-wrap their (empty) parent-lookup maps to match PR-4's Arc-shared ParentLookup signature (semantic merge conflict — textually clean, caught by the crate suite at child head). PR-4's harness-note register entry renumbered F10 -> F11 (F10 was taken by PR-3's star-formation gap; both kept). Gates: fluree-db-query 1261/1261 (nextest); scoped clippy 0; cargo fmt --all --check green.
Contributor
Author
|
Consolidating at maintainer request. #1485 (PR-4 — dimension parent-lookup memoization across child batches) is carried forward in #1507 (the R2RML/Iceberg virtual-dataset performance program), which brings the #1475–#1507 lineage forward as a single reviewable unit. Its verification of record is the program's corpus benchmark, published as C2-bench-wave1.md (native == virtual, 0 hash mismatches). The pre-consolidation branch tip is preserved at tag |
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
1 participant
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.
Roadmap PR-4 (
docs/audit/2026-07-virtual-dataset-perf/ROADMAP.md) — with an honest DoD relabel discovered during gating.What it fixes: the within-operator per-child-batch parent rebuild (original H3)
The correlated R2RML scan rebuilt every RefObjectMap parent-lookup HashMap per 1,000-row child batch (
build_progress), so a fact⋈dim query re-scanned its dimension parents once per batch. Fix: cacheParentLookups across batches on the operator (Arc-shared, keyed byLookupCacheKey), window-capped — a lookup larger than one materialize window is used for its batch but not retained, so a fact-as-parent falls through to today's per-batch build and the cache cannot OOM. Kill switchFLUREE_R2RML_PARENT_MEMO(default on), read per-operator at construction so both regimes are deterministically testable.The on/off scan split is load-immune and self-attributing (only this cache + switch can produce it). q008's hash mismatch is pre-existing (identical at both switch states; F9-class — q008 was DNF at baseline so its hash was never verifiable before). CI enforcement: a scan-counting mock-provider test — 5 child batches ⇒ exactly 1 parent scan memo-on, 5 memo-off — so a future refactor that silently reintroduces the per-batch rebuild fails CI, not just a nightly bench. Query suite 1201/1201.
What it explicitly does NOT fix: q050 (relabeled scope)
The first gate run failed its original DoD: q050 (dims-only correlated OPTIONAL) showed identical scan rates memo on/off. Root cause: the correlated OPTIONAL rebuilds the entire operator tree per correlated row (
build_correlated_optional_op→ seeded rebuild), resetting any operator-scoped cache — this also defeats the existingscan_cache. q050's fix is PR-4b:inner_pattern_is_hash_join_safe(optional.rs:1418) admits onlyTriple/Filter/PropertyPath, and since the R2RML rewrite converts OPTIONAL inners toPattern::R2rml, every OPTIONAL over a virtual dataset falls to per-row rebuild. Widening that gate (soundness analysis for star/multi-valued shapes required) routes the OPTIONAL through the batched hash-left-join — one long-lived inner operator, inside which this PR's memo then applies as designed. The two compose.Diagnosis trail, failed-gate report, and the decision tree are in the session record; the ROADMAP annotation carries the scope split.
Stacks on #1484 (PR-3) ← #1482 ← #1478 ← #1476 ← #1475 ← #1450.