Skip to content

perf(cubeorchestrator): Improve compact transform (-75%, 3.7x-4x)#10830

Merged
ovr merged 1 commit into
masterfrom
perf/query-orchestrator-compact-plan
May 6, 2026
Merged

perf(cubeorchestrator): Improve compact transform (-75%, 3.7x-4x)#10830
ovr merged 1 commit into
masterfrom
perf/query-orchestrator-compact-plan

Conversation

@ovr
Copy link
Copy Markdown
Member

@ovr ovr commented May 6, 2026

The compact transform was the only output path still doing dictionary lookups per cell, per row: for every member it re-walked annotation -> members_to_alias_map -> columns_pos before reading the cell. The other two paths (vanilla, columnar) already precompute a plan.

Bench on my Apple M3 Max TransformedData::transform/compact, median wall time before → after (ms):

cols / rows 1 000 10 000 50 000 100 000
8 0.505 → 0.135 5.11 → 1.35 25.6 → 6.89 51.4 → 13.9
16 1.07 → 0.275 10.5 → 2.72 52.5 → 13.9 104.5 → 28.3
32 2.07 → 0.528 20.6 → 5.36 101.4 → 27.7 203.0 → 55.2
64 4.14 → 1.02 39.9 → 10.3 199.1 → 51.7 401.4 → 101.7
cols / rows 1 000 10 000 50 000 100 000
8 -73.4% -73.6% -72.9% -72.9%
16 -74.3% -74.0% -73.4% -72.9%
32 -74.5% -73.9% -73.2% -72.3%
64 -75.1% -74.1% -73.3% -74.6%

Roughly 3.7-4.0x faster across the matrix. Same magnitude as the analogous hoist on the columnar branch (3849b71f7e).

The compact transform was the only output path still doing dictionary
lookups per cell, per row: for every member it re-walked
annotation -> members_to_alias_map -> columns_pos before reading the
cell. The other two paths (vanilla, columnar) already precompute a plan.

Bench on my Apple M3 Max `TransformedData::transform/compact`, lower = faster:

| cols / rows | 1 000 | 10 000 | 50 000 | 100 000 |
| --- | --- | --- | --- | --- |
|  8 | -73.4% | -73.6% | -72.9% | -72.9% |
| 16 | -74.3% | -74.0% | -73.4% | -72.9% |
| 32 | -74.5% | -73.9% | -73.2% | -72.3% |
| 64 | -75.1% | -74.1% | -73.3% | -74.6% |

Roughly 3.7-4.0x faster across the matrix. Same magnitude as the
analogous hoist on the columnar branch (3849b71f7e).
@github-actions github-actions Bot added the rust Pull requests that update Rust code label May 6, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @ovr's task in 1m 39s —— View job


Code Review: perf(cubeorchestrator) Compact transform plan hoisting

  • Gather context and read the changed file
  • Analyze the diff against origin/master
  • Review code quality, correctness, and performance
  • Review security and edge cases
  • Post detailed review feedback

Overall: This is a clean, well-structured optimization. Approve. The approach is sound — it mirrors the plan-then-execute pattern already established by build_columnar_plan / build_vanilla_plan, eliminating redundant per-row hash-map lookups in favor of a precomputed index vector.

What changed

  1. New types: CompactPlanEntry<'a> (enum with Cell / Constant variants) and CompactPlan<'a> (wrapper struct with entries: Vec).
  2. build_compact_plan — runs once per request, resolving annotation → members_to_alias_map → columns_pos into a flat Vec<CompactPlanEntry>.
  3. get_compact_row — simplified to walk the pre-built plan, doing only a bounds-checked db_row.get(idx) + transform_value per cell.
  4. Call site in TransformedData::transform — builds plan once, maps rows infallibly (no more Result per row).
  5. Tests — all four test functions updated to the two-step build_compact_planget_compact_row pattern, including the multi-row test (rows[1]).

Correctness

  • Behavioral equivalence is preserved. The silent-skip behavior when a lookup fails (annotation miss, alias miss, column miss) is faithfully reproduced: build_compact_plan simply doesn't push an entry for that member, so get_compact_row produces a shorter row — identical to the old code path. Similarly, the db_row.get(*column_index) guard in the new get_compact_row (line 409) preserves the old silent-skip on out-of-bounds, matching prior behavior.
  • BlendingQuery quirk preserved. The comment at line 377-379 correctly notes (and preserves) the original behavior where member_type is resolved from annotation[alias] rather than annotation[member]. This matches the columnar plan at line 582-586.
  • get_compact_row is now infallible (-> Vec<…> instead of -> Result<…>), which is correct: all fallible work (get_date_range_value, get_blending_response_key) moved into build_compact_plan. The call site's .collect::<Result<Vec<_>>>()? correctly simplified to .collect().

Minor observations (non-blocking)

  1. Visibility mismatch: CompactPlan is pub while CompactPlanEntry is pub(crate) and build_compact_plan is pub(crate). Since none of these are used outside the crate (confirmed by grep), CompactPlan could be pub(crate) for consistency with ColumnarColumnPlan (line 529). Very minor.

  2. Silent row shortening on db_row.get() miss (line 409): If column_index is somehow out of bounds for a particular row, that cell is silently dropped, producing a row shorter than plan.entries.len(). This is pre-existing behavior, not introduced here, but worth noting — a debug_assert_eq!(row.len(), plan.entries.len()) after the loop could catch data corruption in dev builds without affecting release perf. Entirely optional.

  3. Clone on Constant variant (line 414): v.clone() for DBResponsePrimitive may allocate for String / Uncommon variants. For CompareDateRangeQuery this runs once per row. The cost is negligible for typical result sets, but if this ever becomes a hot path, the constant could be Rc-wrapped or the value could be a reference. Not actionable now.

Testing

The four existing compact-row tests are correctly adapted to the two-step pattern. Test coverage is adequate for the refactor — each query type variant (Regular, CompareDateRange, Blending) is exercised, including multi-row verification.

Summary

Straightforward plan-hoisting optimization that brings the compact path in line with the vanilla and columnar paths. The ~70% speedup is consistent with removing 3-4 hash lookups per cell. No correctness issues found. LGTM.

@ovr ovr changed the title perf(cubeorchestrator): Improve compact transform (-70%, 3x) perf(cubeorchestrator): Improve compact transform (~ -70%, 3.7x-4x) May 6, 2026
@ovr ovr changed the title perf(cubeorchestrator): Improve compact transform (~ -70%, 3.7x-4x) perf(cubeorchestrator): Improve compact transform (-74%, 3.7x-4x) May 6, 2026
@ovr ovr changed the title perf(cubeorchestrator): Improve compact transform (-74%, 3.7x-4x) perf(cubeorchestrator): Improve compact transform (-75%, 3.7x-4x) May 6, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.82%. Comparing base (7f37faa) to head (2ad2912).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10830      +/-   ##
==========================================
- Coverage   78.83%   78.82%   -0.01%     
==========================================
  Files         470      470              
  Lines       92289    92289              
  Branches     3436     3436              
==========================================
- Hits        72752    72747       -5     
- Misses      19034    19039       +5     
  Partials      503      503              
Flag Coverage Δ
cube-backend 58.15% <ø> (ø)
cubesql 83.47% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ovr ovr merged commit 79f7a70 into master May 6, 2026
95 of 96 checks passed
@ovr ovr deleted the perf/query-orchestrator-compact-plan branch May 6, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants