Skip to content

feat(tesseract): Support use_original_sql_pre_aggregations in rollup#11146

Merged
ovr merged 3 commits into
masterfrom
feat/tesseract-use-original-pre-agg
Jun 23, 2026
Merged

feat(tesseract): Support use_original_sql_pre_aggregations in rollup#11146
ovr merged 3 commits into
masterfrom
feat/tesseract-use-original-pre-agg

Conversation

@ovr

@ovr ovr commented Jun 23, 2026

Copy link
Copy Markdown
Member

No description provided.

@github-actions github-actions Bot added rust Pull requests that update Rust code javascript Pull requests that update Javascript code labels Jun 23, 2026
@ovr ovr marked this pull request as ready for review June 23, 2026 17:20
@ovr ovr requested review from a team as code owners June 23, 2026 17:20
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

No prior review threads. Verdict: LGTM — 0 high, 0 medium, 2 low.

Details (click to expand)
  • Read core Rust diff (query_properties, query_properties_compiler, top_level_planner, bridge)
  • Read JS side (BaseQuery.js, PreAggregations.ts) for context
  • Read integration tests
  • Verify naming/threading consistency

Summary

This PR threads the existing JS-side useOriginalSqlPreAggregationsInPreAggregation flag into the Tesseract (Rust) planner. The behavior mirrors BaseQuery.js:2639 exactly: when set, a rollup-build query also collects originalSql pre-aggregations and substitutes them in, so a rollup is sourced from the cube's originalSql table instead of the raw cube SQL.

The change is small, localized, and well-tested:

  • Bridge struct, compiler, builder, and PartialEq are all updated symmetrically — no field forgotten.
  • The new logic in top_level_planner.rs is just an OR-extension of the existing !is_pre_aggregation_query() gate — minimal blast radius.
  • Two new integration tests (positive + negative) directly assert the SQL substitution effect, plus a separate lambda_union test for an adjacent fix.
  • Test fixtures (MockBaseQueryOptions, YamlBaseQueryOptions, TestContext) are updated; bridge coverage test gets the new field added.

Code quality

  • The doc comment on query_properties.rs:170-172 is well-scoped and explains the why, not the what — good.
  • The inline comment in top_level_planner.rs:56-58 is similarly useful for explaining the OR condition.
  • Naming matches the JS source-of-truth (useOriginalSqlPreAggregationsInPreAggregationuse_original_sql_pre_aggregations_in_pre_aggregation); long but precedent-consistent.

Minor observations (low)

  1. top_level_planner.rs:53 — the blank line between the let templates = … statement and the PhysicalPlanBuilder::new line was removed in this diff. Not wrong, but it was an unrelated formatting change; small nit.

  2. Test naming test_rollup_lambda_cross_cube_union_aliases in sql_generation.rs is unrelated to the PR title (it's for a lambda_union matcher concern, not useOriginalSqlPreAggregations). Worth either splitting into a separate PR or noting in the PR description, since the title says only "Support use_original_sql_pre_aggregations in rollup." Not blocking.

Security / performance

  • No security surface touched (boolean propagation only).
  • No new query paths; the same OriginalSqlCollector is reused. No perf concern.

Tests

  • Coverage looks solid: positive case asserts foo__main appears and foo_table does not; negative case asserts the inverse. Both assertions on both directions — robust against accidental no-op regressions.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 43s —— View job


Code Review (re-run after sync)

Verdict: LGTM — 0 high, 0 medium, 1 low.

  • Diff vs origin/master re-read
  • Verified chore: drop test removed the unrelated test_rollup_lambda_cross_cube_union_aliases flagged previously
  • Verified chore: fix build added the new field to object-bridges-coverage.test.ts
  • Re-checked Rust threading (bridge → compiler → properties → planner) for symmetry
Details (click to expand)

Summary

This PR threads the existing JS-side useOriginalSqlPreAggregationsInPreAggregation flag into the Tesseract (Rust) planner. Behavior mirrors BaseQuery.js:2639: when set, a rollup-build query also collects originalSql pre-aggregations and substitutes them in, so a rollup is sourced from the cube's originalSql table instead of the raw cube SQL.

Since the previous review:

  • The unrelated test_rollup_lambda_cross_cube_union_aliases test was removed (152fc43), so the diff now matches the PR title scope.
  • object-bridges-coverage.test.ts was updated (1f2f2b5) to include the new field in the bridge coverage list — required for that test to pass.

The change is small, localized, well-tested:

  • Bridge struct, compiler, builder, PartialEq, and the MockBaseQueryOptions/YamlBaseQueryOptions/TestContext fixtures are all updated symmetrically — no field forgotten.
  • The new logic in top_level_planner.rs is just an OR-extension of the existing !is_pre_aggregation_query() gate — minimal blast radius.
  • Two integration tests (positive + negative) directly assert the SQL substitution effect.

Code quality

  • Doc comment on query_properties.rs:170-172 explains the why, not the what — good.
  • Inline comment in top_level_planner.rs:56-58 similarly useful for explaining the OR condition.
  • Naming consistent with the JS source-of-truth (useOriginalSqlPreAggregationsInPreAggregationuse_original_sql_pre_aggregations_in_pre_aggregation).

Minor observation (low)

  1. top_level_planner.rs:53 — the blank line between the let templates = … statement and PhysicalPlanBuilder::new was removed. Unrelated formatting nit; not blocking.

Security / performance

  • No security surface touched (boolean propagation only).
  • No new query paths; same OriginalSqlCollector is reused. No perf concern.

Tests

  • Coverage looks solid: positive case asserts foo__main appears and foo_table does not; negative case asserts the inverse. Both directions covered — robust against accidental no-op regressions.
• [Branch](https://github.com/cube-js/cube/tree/feat/tesseract-use-original-pre-agg)

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.19%. Comparing base (e317395) to head (152fc43).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11146      +/-   ##
==========================================
- Coverage   83.68%   79.19%   -4.50%     
==========================================
  Files         257      474     +217     
  Lines       79384    96895   +17511     
  Branches        0     3563    +3563     
==========================================
+ Hits        66434    76732   +10298     
- Misses      12950    19645    +6695     
- Partials        0      518     +518     
Flag Coverage Δ
cube-backend 58.80% <ø> (?)
cubesql 83.68% <ø> (ø)

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

☔ View full report in Codecov by Harness.
📢 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 6a640a6 into master Jun 23, 2026
95 of 97 checks passed
@ovr ovr deleted the feat/tesseract-use-original-pre-agg branch June 23, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants