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

[MBQL lib] Cache round trips between legacy and lib in both directions #45865

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

Conversation

bshepherdson
Copy link
Contributor

Description

Previously, conversions from legacy to MLv2 were cached based on the identity of the incoming query.

This PR adds new caching for round trips in both directions:

  1. When converting to MLv2, the legacy form is saved in the Clojure metadata. When converting back that value is used
    unless the query has been edited.
  2. When converting to legacy, the MLv2 form is saved in a non-enumerable JS property. When converting back to MLv2,
    that value is used unless the metadata-provider has changed.

See the comments in metabase.lib.js/query and legacy-query for more details.

NOTE All of this refers only to the CLJS side.

How to verify

This should be invisible, except for the speedup. Put a breakpoint in the inner conversion functions like
legacy-query* to check if they're called on a round trip.

Checklist

  • Tests have been added/updated to cover changes in this PR

Copy link
Contributor Author

bshepherdson commented Jul 19, 2024

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

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

Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

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

Awesome!

@bshepherdson bshepherdson force-pushed the mblib-perf-cache-more-legacy-conversions branch from 5c9cf70 to 2f2e4e1 Compare July 19, 2024 16:26
@bshepherdson bshepherdson changed the base branch from perf-dashboard-load-id-memo-fix to master July 19, 2024 16:26
Copy link

replay-io bot commented Jul 19, 2024

Status Complete ↗︎
Commit f6279aa
Results
10 Failed
⚠️ 5 Flaky
2817 Passed

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.

LGTM - Slightly worried that making it easier to use legacy queries removes the impetus to move forward with pmbql in the app.

@@ -680,3 680,119 @@
:keyword "too"
:value nil}]
(is (js= expected (lib.js/display-info->js input))))))

(deftest ^:parallel fresh-legacy-query-caching-test
Copy link
Contributor

@lbrdnk lbrdnk Jul 19, 2024

Choose a reason for hiding this comment

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

This test (and others) is marked parallel while using with-redefs. I suppose that could be an issue in clj. Is that ok in cljs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. I updated these tests to ^:synchronized. (Though as you say, it doesn't matter in JS.) I also added cljs.core/with-redefs and friends to the linter's list of things disallowed in parallel tests. In :require etc. namespace that start with clojure.* get rewritten to cljs.* but that doesn't happen in the linter.

@bshepherdson bshepherdson force-pushed the mblib-perf-cache-more-legacy-conversions branch from 2f2e4e1 to 8ab8a5b Compare July 19, 2024 17:21
@bshepherdson bshepherdson added the no-backport Do not backport this PR to any branch label Jul 19, 2024
@bshepherdson bshepherdson enabled auto-merge (squash) July 19, 2024 17:22
@bshepherdson bshepherdson force-pushed the mblib-perf-cache-more-legacy-conversions branch from f6279aa to 73c2b0d Compare July 26, 2024 19:25
…46104)

* Number formatter Reports correct Decimal places for negative Numbers

Fixes #41640

Prior to this, the number formatter would get the incorrect number of decimal places for negative numbers.

The way the decimal places count is derived works as follows:

 - convert number to a string
 - split the string on the separator (decimal, comma, whatever the instance has set)
 - destructure that split into [int decimal]
 - count decimal to get the number of decimals.

That all works for positive numbers, but for negative numbers, the regex was just splitting on 'not digits', so we got
an extra group due to the leading negative sign. This would incorrectly destructure  the 'int' part of the number into
the count, thus we would always get 1 instead of the correct value.

This fixes it by just taking last instead of destructuring.

* correct comparison to always use abs value in conditonal check

* tiny incorrectness in the test
@bshepherdson bshepherdson force-pushed the mblib-perf-cache-more-legacy-conversions branch from 73c2b0d to 9bf471e Compare August 27, 2024 13:50
See the comments in `metabase.lib.js` about the various approaches to
caching.
@bshepherdson bshepherdson force-pushed the mblib-perf-cache-more-legacy-conversions branch from 9bf471e to 789fe5e Compare August 27, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor(deprecated) Use .Team/Querying instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants