-
Notifications
You must be signed in to change notification settings - Fork 5k
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
base: master
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bshepherdson and the rest of your teammates on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
5c9cf70
to
2f2e4e1
Compare
|
There was a problem hiding this 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.
test/metabase/lib/js_test.cljs
Outdated
@@ -680,3 680,119 @@ | |||
:keyword "too" | |||
:value nil}] | |||
(is (js= expected (lib.js/display-info->js input)))))) | |||
|
|||
(deftest ^:parallel fresh-legacy-query-caching-test |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
2f2e4e1
to
8ab8a5b
Compare
f6279aa
to
73c2b0d
Compare
…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
73c2b0d
to
9bf471e
Compare
See the comments in `metabase.lib.js` about the various approaches to caching.
9bf471e
to
789fe5e
Compare
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:
unless the query has been edited.
that value is used unless the
metadata-provider
has changed.See the comments in
metabase.lib.js/query
andlegacy-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