Skip to content

Conversation

@mcheshkov
Copy link
Contributor

@mcheshkov mcheshkov commented Apr 4, 2025

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

Description of Changes Made (if issue reference is not provided)

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous for each member.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches

Contains #9429, I want to land that separately

@mcheshkov mcheshkov force-pushed the pre-agg-join-path branch 2 times, most recently from e267565 to 567a9f8 Compare April 7, 2025 11:18
@mcheshkov mcheshkov changed the title [WIP] fix(schema-compiler): Use join paths from pre-aggregation declaration instead of building join tree from scratch fix(schema-compiler): Use join paths from pre-aggregation declaration instead of building join tree from scratch Apr 7, 2025
@mcheshkov mcheshkov force-pushed the pre-agg-join-path branch 2 times, most recently from 293e1a7 to 85d23eb Compare April 7, 2025 11:26
@mcheshkov mcheshkov marked this pull request as ready for review April 7, 2025 11:27
@mcheshkov mcheshkov requested review from a team as code owners April 7, 2025 11:27
@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

Attention: Patch coverage is 70.83333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 47.92%. Comparing base (14ae6a4) to head (ed746ef).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...es/cubejs-schema-compiler/src/adapter/BaseQuery.js 55.55% 7 Missing and 1 partial ⚠️
...ejs-schema-compiler/src/adapter/PreAggregations.js 62.50% 3 Missing ⚠️
...cubejs-schema-compiler/src/compiler/CubeSymbols.ts 50.00% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (14ae6a4) and HEAD (ed746ef). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (14ae6a4) HEAD (ed746ef)
cubesql 2 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #9431       +/-   ##
===========================================
- Coverage   76.57%   47.92%   -28.65%     
===========================================
  Files         400      171      -229     
  Lines      105044    21424    -83620     
  Branches     3707     3710        +3     
===========================================
- Hits        80434    10268    -70166     
+ Misses      24174    10718    -13456     
- Partials      436      438        +2     
Flag Coverage Δ
cube-backend 47.92% <70.83%> (+0.06%) ⬆️
cubesql ?

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 requested review from KSDaemon and waralexrom April 8, 2025 11:35
@mcheshkov mcheshkov force-pushed the pre-agg-join-path branch from 85d23eb to 04ed609 Compare April 8, 2025 13:40
… instead of building join tree from scratch

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches
@mcheshkov mcheshkov force-pushed the pre-agg-join-path branch from 04ed609 to ed746ef Compare April 8, 2025 17:12
@mcheshkov mcheshkov merged commit aea3a6c into master Apr 8, 2025
69 checks passed
@mcheshkov mcheshkov deleted the pre-agg-join-path branch April 8, 2025 18:16
mcheshkov added a commit that referenced this pull request Apr 10, 2025
…laration instead of building join tree from scratch (#9431)"

This reverts commit aea3a6c.
mcheshkov added a commit that referenced this pull request Apr 10, 2025
…laration instead of building join tree from scratch (#9431)" (#9448)

This reverts commit aea3a6c, PR #9431
It contains a bug in pre-aggregation matching, rendering any pre-agg with join path unusable
mcheshkov added a commit that referenced this pull request Apr 15, 2025
… instead of building join tree from scratch (#9431)

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches

(cherry picked from commit aea3a6c)
mcheshkov added a commit that referenced this pull request Apr 22, 2025
… instead of building join tree from scratch (#9431)

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches

(cherry picked from commit aea3a6c)
mcheshkov added a commit that referenced this pull request Apr 22, 2025
… instead of building join tree from scratch (#9431)

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches

(cherry picked from commit aea3a6c)
marianore-muttdata pushed a commit to MuttData/cube that referenced this pull request Jun 17, 2025
… instead of building join tree from scratch (cube-js#9431)

Without this pre-aggregatoin declaration would build a list of members, and build join tree from scratch. New join tree could be different, and pre-aggregation would contain incorrect data.

Add join hints to member adapters, to build join tree with proper members. This is more of a hack than proper solution: join tree would still disallow to have same cube twice with different join paths.
Just adding extra join hints on query level does not work because of FKQA and multiplication check in subqueries: we have to know join path for every member, so it should be either explicit or unambigous _for each member_.

Join tree from query is still not included in pre-aggregation matching, so user would have to manually ensure that views and pre-aggregations both match each other and does not produce false matches
marianore-muttdata pushed a commit to MuttData/cube that referenced this pull request Jun 17, 2025
…laration instead of building join tree from scratch (cube-js#9431)" (cube-js#9448)

This reverts commit aea3a6c, PR cube-js#9431
It contains a bug in pre-aggregation matching, rendering any pre-agg with join path unusable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants