feat(tesseract): Basic pre-aggregations support #9434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

waralexrom wants to merge 13 commits into master from tesseract-basic-pre-aggregations

Member

waralexrom commented Apr 7, 2025 •

edited by KSDaemon

Loading

Check List

Tests have been run in packages where changes made if available
Linter has been run for changed code
Tests for the changes have been added if not covered yet
Docs have been added / updated if required

codecov bot commented Apr 7, 2025 •

edited

Loading

Codecov Report

Attention: Patch coverage is 19.35484% with 50 lines in your changes missing coverage. Please review.

Project coverage is 80.27%. Comparing base (183c35a) to head (d5fd502).

Files with missing lines	Patch %	Lines
...es/cubejs-schema-compiler/src/adapter/BaseQuery.js	18.75%	39 Missing ⚠️
...ejs-schema-compiler/src/adapter/PreAggregations.js	23.07%	10 Missing ⚠️
...bejs-schema-compiler/src/compiler/CubeEvaluator.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9434      +/-   ##
==========================================
- Coverage   83.96%   80.27%   -3.70%     
==========================================
  Files         230      395     +165     
  Lines       84004    97550   +13546     
  Branches        0     2278    +2278     
==========================================
+ Hits        70536    78309    +7773     
- Misses      13468    18933    +5465     
- Partials        0      308     +308

Flag	Coverage Δ
cube-backend	`57.38% <19.35%> (?)`
cubesql	`83.96% <ø> (ø)`

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.

waralexrom force-pushed the tesseract-basic-pre-aggregations branch from be6e357 to ccf4c02 Compare

April 21, 2025 18:11

waralexrom changed the base branch from master to tesseract-logical-plan

April 21, 2025 18:12

waralexrom force-pushed the tesseract-logical-plan branch from 2d7ac7e to 37e2479 Compare

April 28, 2025 08:37

Base automatically changed from tesseract-logical-plan to master

April 28, 2025 09:33

waralexrom force-pushed the tesseract-basic-pre-aggregations branch from ccf4c02 to 09309d0 Compare

April 28, 2025 18:43

waralexrom marked this pull request as ready for review

May 1, 2025 14:20

waralexrom requested a review from a team as a code owner

May 1, 2025 14:20

waralexrom added 12 commits

May 1, 2025 16:22


          feat(tesseract): Basic pre-aggregations support

a2bd973


          in work

db0f317


          in work

caa0ca9


          in work

60131c2


          in work

7042ec4


          in work

31d7533


          in work

cf2d8f2


          in work

a93a7cd


          in work


          in work

1ff51d2

fix

da3271b

fix

335c2ed

waralexrom force-pushed the tesseract-basic-pre-aggregations branch from 995c326 to 335c2ed Compare

May 1, 2025 14:51


          lint

d5fd502

waralexrom requested a review from KSDaemon

May 1, 2025 16:56

KSDaemon reviewed

View reviewed changes

Member

KSDaemon left a comment

🚀 This is great!

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

+                    const hasMultiStageMeasures = this.fullKeyQueryAggregateMeasures({ hasMultipliedForPreAggregation: true }).multiStageMembers.length > 0;
+                    this.canUseNativeSqlPlannerPreAggregation = hasMultiStageMeasures;
+                  } else {
+                    this.useNativePreAggregations = false;

Member

KSDaemon May 2, 2025

Is this property set in other cases? It's a class property and should be initialized in the constructor or at least defined. I know, this is JS which never complains, but let's follow a more right approach.

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

@@ @@ -289,6 +289,12 @@ export class BaseQuery { @@
                   }).filter(R.identity).map(this.newTimeDimension.bind(this));
                   this.allFilters = this.timeDimensions.concat(this.segments).concat(this.filters);
                   this.useNativeSqlPlanner = this.options.useNativeSqlPlanner ?? getEnv('nativeSqlPlanner');
+                  if (this.useNativeSqlPlanner) {
+                    const hasMultiStageMeasures = this.fullKeyQueryAggregateMeasures({ hasMultipliedForPreAggregation: true }).multiStageMembers.length > 0;
+                    this.canUseNativeSqlPlannerPreAggregation = hasMultiStageMeasures;

Member

KSDaemon May 2, 2025

Is this property set in other cases? It's a class property and should be initialized in the constructor or at least defined. I know, this is JS which never complains, but let's follow a more right approach.

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

@@ @@ -471,6 +477,19 @@ export class BaseQuery { @@
                 }
                 newDimension(dimensionPath) {
+                  if (dimensionPath instanceof String || typeof dimensionPath === 'string') {

Member

KSDaemon May 2, 2025

Suggested change

      
                if (dimensionPath instanceof String || typeof dimensionPath === 'string') {
          
                if (typeof dimensionPath === 'string') {

There is no need in instanceof check. Because instanceof String catches only objects created as new String('abc'), and such objects are almost never used in practice.

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

@@ @@ -471,6 +477,19 @@ export class BaseQuery { @@
                 }
                 newDimension(dimensionPath) {
+                  if (dimensionPath instanceof String || typeof dimensionPath === 'string') {
+                    const memberArr = Array.isArray(dimensionPath) ? dimensionPath : dimensionPath.split('.');

Member

KSDaemon May 2, 2025

dimensionPath will never be an Array here, because you just checked it for a string! %)

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

+                          this.cubeEvaluator.isDimension(memberArr.slice(0, -2))) {
+                      return this.newTimeDimension(
+                        {
+                          dimension: this.cubeEvaluator.pathFromArray(memberArr.slice(0, -2)),

Member

KSDaemon May 2, 2025

I think you don't need to call cubeEvaluator.pathFromArray here as you already have a dimensionPath as an array. it's a useless turnaround.

rust/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs

+                      rewrited_multistage: &mut HashMap<String, bool>,
+                      pre_aggregation: &Rc<CompiledPreAggregation>,
+                  ) -> Result<(), CubeError> {
+                      if let Some(rewrited) =

Member

KSDaemon May 2, 2025

Suggested change

      
                    if let Some(rewrited) =
          
                    if let Some(rewritten) =

rust/cubesqlplanner/cubesqlplanner/src/logical_plan/optimizers/pre_aggregation/optimizer.rs

+                      }
+                  }
+                  fn is_schema_and_filters_match(

Member

KSDaemon May 2, 2025

Suggested change

      
                fn is_schema_and_filters_match(
          
                fn do_schema_and_filters_match(

rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/collectors/cube_names_collector.rs

                   visitor.apply(node, &())?;
                   Ok(visitor.extract_result())
               }
+              pub fn collect_cube_names_from_vec(

Member

KSDaemon May 2, 2025

Suggested change

      
            pub fn collect_cube_names_from_vec(
          
            pub fn collect_cube_names_from_members(

rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/symbols/dimension_symbol.rs

		definition: Rc<dyn DimensionDefinition>,
		is_reference: bool, // Is symbol is direct reference to another symbol without any calculations

Member

KSDaemon May 2, 2025

Suggested change

      
                is_reference: bool, // Is symbol is direct reference to another symbol without any calculations
          
                is_reference: bool, // Symbol is a direct reference to another symbol without any calculations

rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/symbols/dimension_symbol.rs

@@ @@ -241,6 +247,13 @@ impl SymbolFactory for DimensionSymbolFactory { @@
                       } else {
                           None
                       };
+                      let is_sql_is_direct_ref = if let Some(sql) = &sql {

Member

KSDaemon May 2, 2025

Suggested change

      
                    let is_sql_is_direct_ref = if let Some(sql) = &sql {
          
                    let is_sql_direct_ref = if let Some(sql) = &sql {

KSDaemon requested changes

View reviewed changes

Member

KSDaemon left a comment

🚀 This is great! However there are some places need fixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet