Repository navigation
#433 — single-pass config provenance + var-inventory package precompute + filterByModels lookup dedup - #441
Conversation
…recompute + filterByModels lookup dedup Three correctness-neutral traversal/lookup dedups on the #262 project-config surfaces (deferred from #432): - provenance_for_fqn (src/domain/project_def.rs): one fqn descent accumulating deepest-match-wins configs (O(K·L) -> O(L)); removes the keys_along_fqn helper. A BTreeMap accumulator keeps the output sorted by key, so the rendered ordering is byte-identical to the prior pass. - project_var_inventory (src/domain/vars.rs): resolve each scanned model's package once (O(M)) instead of per var (O(V×M) manifest lookups + a NodeId clone each); inventory_entry now takes the precomputed map. - filterByModels (templates/explore-lineage.js): resolve each Cytoscape element once, not 3× per id; behavior + zero-egress unchanged. Behavior-neutral by construction (same key set / order / values; same scope-reach + tallies; same matched ids). Every committed golden is byte-identical except the four explore pages embedding the changed JS, regenerated here. Per the issue AC these are justified by complexity reduction + the provenance maintainability win (simpler algorithm, one fewer helper); no profiler or perf budget exists for this zero-compute tool, so the perf framing is analytical Big-O, not benchmarked. Closes #433 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThree redundant-traversal hotspots are addressed. ChangesfilterByModels Cytoscape Lookup Dedup
Rust Domain Single-Pass and Precompute Refactors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)examples/explore-macro/dag.htmlexamples/explore-macro/macro.htmlexamples/explore-seed/dag.html
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch 🧭 Explore previewThe two-page 🟡 Golden exploreThe committed
🐶 Live exploreThis PR doesn't touch ▶ Open ↗ opens the report or explorer in your browser in one The Pages preview may take ~1 min to update after this comment Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 27558780877 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations by caching and precomputing lookups. In the HTML and JS visualization files, Cytoscape node resolutions are cached to avoid redundant lookups. In the Rust backend, FQN config resolution is optimized to a single descent, and model package resolutions are precomputed to avoid repeated manifest lookups. The review feedback suggests further optimizing the Rust package resolution by using a flat vector instead of a BTreeMap, which avoids O(log M) map lookups in the inner loop and reduces the overall complexity to O(V * M).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…a map lookup gemini-code-assist flagged the var-inventory precompute on PR #441: pairing each scanned model with its resolved package in a flat Vec lets inventory_entry iterate the slice directly, dropping the per-iteration O(log M) BTreeMap lookup (O(V×M log M) -> O(V×M)) and collapsing two params into one. Behavior-neutral (same iteration order / tallies / var_models); the explore project-pane goldens are byte-identical (re-rendered + diffed, verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merged — debriefShipped (squash Deviation (review-driven, intentional): the var-inventory precompute first used a Bot disposition: gemini — 2 medium threads, both real (probed before disposition) → adopted + replied + resolved. CodeRabbit — Review completed, no findings. Gates: all green locally (fmt/clippy AC note: the "no measured baseline" AC was honored via complexity-reduction + the Risks / follow-ups: none — behavior-neutral, no output change, no new surface. |
1 participant
Summary
Post-v0.1 perf pass for the three redundant-traversal/lookup hotspots gemini flagged on #432 (#270 explorer project pane). All three are correctness-neutral.
provenance_for_fqn(src/domain/project_def.rs) — one fqn descent accumulating deepest-match-wins configs (O(K·L)→O(L)), removing thekeys_along_fqnhelper. ABTreeMapaccumulator keeps the output sorted by key, so the rendered ordering is byte-identical to the priorkeys_along_fqn+ per-key re-resolve. (Carries the maintainability win the issue calls out — simpler algorithm, one fewer helper.)project_var_inventory(src/domain/vars.rs) — resolve each scanned model's package once (O(M)) instead of per var (O(V×M)manifest lookups + aNodeIdclone each).inventory_entrynow takes the precomputedmodel_pkgsmap; the public API is unchanged.filterByModels(templates/explore-lineage.js) — resolve each Cytoscape element once into a{id, node}list instead of callingcy.getElementByIdup to 3× per id (filter, union, select). Behavior + zero-egress unchanged.Behavior-neutral by construction
BTreeMap), same deepest(depth, value)→ samepath.package_name().or(root)), samescope_reachesgate, same per-tier tallies, samevar_modelspushes/order.presentmembership + order;activeFilter.matchedis the same array of ids (present.map(p => p.id)).Every committed golden is byte-identical except the four explore pages that embed the changed JS (
explore/dag.html,explore-macro/dag.html,explore-macro/macro.html,explore-seed/dag.html), regenerated here — the diff in each is only thefilterByModelsregion. The unchangedtests.htmlpages + all report goldens independently confirm the Rust changes are byte-neutral.On the "don't optimize without a measured baseline" AC
cute-dbt is zero-compute on typically small dbt artifacts and has no established perf budget (the org's aspirational
—Q4 leg). So per the issue's AC alternative, the justification here is complexity reduction + theprovenance_for_fqnmaintainability win, not a profiler-driven hot-path fix — the perf framing is analytical Big-O, not benchmarked. Nocriterioninfra added (it isn't a current dep; adding it solely for this would be scope creep against a zero-compute tool with no regression ceiling).Gates (all green locally)
cargo fmt --check✓ ·cargo clippy --all-targets --locked -- -D warnings✓ ·RUSTDOCFLAGS=-D warnings cargo doc --no-deps --locked✓ ·cargo deny check✓cargo nextest run --locked→ 2367 passed (incl. the headless explore-lineage tests that drivefilterByModels)cargo test --test bdd→ 31 features / 242 scenarios passedDO NOT MERGE
Christopher's call. Gated to green + left open for review.
Closes #433
🤖 Generated with Claude Code
Summary by CodeRabbit