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.

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

[MLv2] Introduce swap-clauses to reorder aggregations, etc. #39850

Merged
merged 1 commit into from
Mar 11, 2024
Merged

[MLv2] Introduce swap-clauses to reorder aggregations, etc. #39850

merged 1 commit into from
Mar 11, 2024

Conversation

bshepherdson
Copy link
Contributor

This includes some generative testing, since that felt like
a great way to try all kinds of permutations of swapping clauses.

This also patches metabase.types/assignable? to memoize it.
Deeply nested assignable? calls made 100 tests of swapping
filters take 1600ms rather than the 50-60ms of the other clauses.

I think this is safe and reasonable on memory to cache forever,
but speak up if you're concerned. There's perhaps O(60) type
keywords, so type X type -> Boolean is a small space impact.

@bshepherdson Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bshepherdson and the rest of your teammates on Graphite Graphite

@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Mar 8, 2024
@bshepherdson bshepherdson added the no-backport Do not backport this PR to any branch label Mar 8, 2024
@bshepherdson bshepherdson added this to the 0.50 milestone Mar 8, 2024
@replay-io replay.io
Copy link

replay-io bot commented Mar 8, 2024 •
edited

Status Complete ↗︎
Commit 469fb1d
Results
⚠️ 1 Flaky
✅ 2352 Passed

snoe
snoe approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@snoe snoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe order can be dependent in expressions & joins. I'm not sure if the FE is requesting these swaps or not, if not then I would leave them out of swappable causes for the moment, if so then I would prevent illegal swaps.

test/metabase/lib/swap_test.cljc Show resolved Hide resolved
test/metabase/lib/swap_test.cljc Outdated Show resolved Hide resolved
test/metabase/lib/swap_test.cljc Show resolved Hide resolved
@ranquild
Copy link
Contributor

ranquild commented Mar 11, 2024 •
edited

I believe order can be dependent in expressions & joins. I'm not sure if the FE is requesting these swaps or not, if not then I would leave them out of swappable causes for the moment, if so then I would prevent illegal swaps.

Currently the FE would not call swapClauses with joins.

@ranquild
Copy link
Contributor

ranquild commented Mar 11, 2024 •
edited

FE changes were approved here #39205

@bshepherdson bshepherdson enabled auto-merge (squash) March 11, 2024 21:41
@bshepherdson bshepherdson merged commit 796256c into master Mar 11, 2024
107 checks passed
@bshepherdson bshepherdson deleted the mblib-swap-clauses branch March 11, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snoe snoe snoe approved these changes

@ranquild ranquild ranquild approved these changes

@camsaul camsaul Awaiting requested review from camsaul camsaul is a code owner

@metamben metamben Awaiting requested review from metamben

Assignees

@bshepherdson bshepherdson

Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Milestone
0.50
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bshepherdson @ranquild @snoe