Skip to content
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.

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

fix(cubestore): don't add all measures to default preagg index #8789

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KSDaemon
Copy link
Member

@KSDaemon KSDaemon commented Oct 7, 2024

This change improves the cube store preaggregation creation: the default index for a preaggregation now excludes all measures from the sorting keys

Check List

  • Tests has 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

Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-angular-dashboard ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-d3 ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-dashboard ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-data-table ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-highcharts ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-material-ui ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-react-pivot-table ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm
examples-vue-query-builder ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 2:45pm

@KSDaemon KSDaemon force-pushed the cube-store-no-deflt-idx-measures branch from 7f09431 to a2ce1e5 Compare October 7, 2024 16:52
@igorlukanin
Copy link
Member

@KSDaemon I believe this has to be changed: https://cube.dev/docs/product/caching/using-pre-aggregations#using-indexes

Should this be considered a breaking change? Also, will upgrading to this version result in pre-aggregations being rebuilt, entirely or partially?

@KSDaemon KSDaemon marked this pull request as ready for review October 8, 2024 12:50
@KSDaemon KSDaemon requested a review from a team as a code owner October 8, 2024 12:50
@KSDaemon
Copy link
Member Author

KSDaemon commented Oct 8, 2024

@igorlukanin Actually No :) It's internal optimization, so indexes are still copies of all preagg data. The difference is that now, sorting keys for the index do not include measures (as it's mostly useless, no filtering is done by measures, potentially they may be useful for postagregation filtering (aka HAVING xxx). But that is another story.

Rgd existing indexes — there should be no changes. This technique will be applied only for new index creation.

@igorlukanin
Copy link
Member

Docs still need to be updated wrt what is included in the default index, don't they?

@KSDaemon
Copy link
Member Author

KSDaemon commented Oct 8, 2024

When you define a pre-aggregation without any explicit indexes, the default index is created. In this index, dimensions come first, time dimensions come second, and measures come last.

This is still true :)

@igorlukanin
Copy link
Member

@KSDaemon Hopefully I did it correctly.

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.

4 participants