MLv2 Move metadata calculation functions into respective namespaces #29245

camsaul · 2023-03-15T17:29:14Z

Split off from #29200

This is the Clojure-only side of #29200, I think it makes sense to try to get this stuff in now while I iron out the bugs in some of the e2e tests to avoid more merge conflicts in the future.

After feedback in this Slack thread https://metaboat.slack.com/archives/C04DN5VRQM6/p1678742327970719, I shuffled around the multimethods used for metadata calculation display-name, column-name, etc. and moved their implementations to live with the other functions related to them. E.g. the display-name method for aggregations lives in metabase.lib.aggregations now, instead of in metabase.lib.metadata.calculate.names. About 90% of this PR is just moving stuff around so it's easier to deal with going forward.
I simplified some of the metadata calculation stuff so it's now done with a smaller number of core multimethods
Tweaks to the metadata provider protocol that I will call out in the comments
General util functions I will call out in the comments

camsaul · 2023-03-15T17:30:08Z

.clj-kondo/config.edn

+    :single-key-in                                         {:level :warning}
+    :keyword-binding                                       {:level :warning}
+    :shadowed-var                                          {:level :warning}
+    :metabase/deftest-not-marked-parallel-or-synchronized  {:level :warning}}}}}


I added a custom Kondo linter to require that metabase.lib.* tests are marked either ^:parallel or ^:synchronous if for some reason you don't want them to be parallel. Hopefully this will help us remember to make them all ^:parallel and thus fast

camsaul · 2023-03-15T17:30:46Z

src/metabase/lib/aggregation.cljc

+                       :stage-number stage-number})))
+    (nth aggregations index)))
+
+(defmethod lib.metadata.calculation/describe-top-level-key :aggregation


Clojure(Script) impl of query description logic

camsaul · 2023-03-15T17:31:21Z

src/metabase/lib/aggregation.cljc

   [metabase.util.malli :as mu])
  #?(:cljs (:require-macros [metabase.lib.aggregation])))

+(mu/defn resolve-aggregation :- ::lib.schema.aggregation/aggregation


Moved some of the existing code that was in metabase.lib.metadata.calculate.resolve to places that actually need/use it

camsaul · 2023-03-15T17:32:35Z

src/metabase/lib/aggregation.cljc

+     (for [aggregation aggregations]
+       (lib.metadata.calculation/display-name query stage-number aggregation)))))
+
+(defmethod lib.metadata.calculation/metadata :aggregation


All this stuff is already in, I just moved it over from the old namespaces

camsaul · 2023-03-15T18:00:17Z

src/metabase/lib/js/metadata.cljs

@@ -0,0 +1,150 @@
+(ns metabase.lib.js.metadata


JavaScript implementation of a MetadataProvider that works with the metadata available to Question.ts

camsaul · 2023-03-15T18:03:31Z

src/metabase/lib/metadata/protocols.cljc

-(#?(:clj p/defprotocol+ :cljs defprotocol) DatabaseMetadataProvider
+(#?(:clj p/defprotocol+ :cljs defprotocol) MetadataProvider


I reworked the DatabaseMetadataProvider protocol to include a lot more stuff that was on the TODO-list, for example methods to fetch specific tables by ID and other things that we're going to need eventually when we implement a QP store version of the provider or try to hook it in to the Redux store if we end up doing that

I added methods for Metrics and Segments too which we will need sooner rather than later. Metadata.ts already has them. Since this isn't just DB stuff anymore I renamed it to MetadataProvider

camsaul · 2023-03-15T18:04:15Z

src/metabase/lib/stage.cljc

+
+(declare stage-metadata)
+
+(mu/defn ^:private breakout-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]


All this stuff is just moved over from metabase.lib.metadata.calculate

camsaul · 2023-03-15T18:04:51Z

src/metabase/lib/util.cljc

+   [metabase.util.malli :as mu]
+   #?@(:clj
+       ([potemkin :as p]))
+   #?@(:cljs
+       ([goog.string :as gstring]
+        [goog.string.format :as gstring.format]))))


I finally added metabase.lib.util/format so we can use it in both Clojure and ClojureScript

camsaul · 2023-03-15T18:05:24Z

test/metabase/lib/aggregation_test.cljc

+  ([table field options]
+   [:field (merge {:lib/uuid (str (random-uuid))} options) (meta/id table field)]))
+
+(deftest ^:parallel aggregation-names-test


Most of these tests already existed, I just moved them so they live in the same namespace as the clauses they're testing

camsaul · 2023-03-15T18:14:24Z

test/metabase/lib/test_util.cljc

@@ -0,0 +1,64 @@
+(ns metabase.lib.test-util


Added new test util namespace for stuff I found myself using in a bunch of different tests.

camsaul · 2023-03-15T18:14:51Z

test/metabase/lib/test_util.cljc

+           options)
+    (meta/id table field)]))
+
+(defn mock-metadata-provider


Mock metadata provider for those rare cases when we need to test stuff not covered by test-metadata/provider, e.g. nested columns

camsaul · 2023-03-15T18:29:40Z

src/metabase/lib/aggregation.cljc

+
+;;; TODO -- merge this stuff into `defop` somehow.
+
+(defmethod lib.metadata.calculation/display-name-method :aggregation


display-name-method here rhymes with clojure.core/print-method. This was previously called display-name* but I decided this was more meaningful and makes it clearer when you should use display-name (the wrapper function with error handling and type checking) vs display-name-method... display-name-method makes it clearer that you should implement display-name-method but call display-name than before.

codecov · 2023-03-15T18:39:31Z

Codecov Report

Patch coverage: 64.20% and project coverage change: -0.03 ⚠️

Comparison is base (738ec74) 68.48% compared to head (49c96e0) 68.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29245      +/-   ##
==========================================
- Coverage   68.48%   68.45%   -0.03%     
==========================================
  Files        2785     2788       +3     
  Lines       96184    96434     +250     
  Branches    12290    12298       +8     
==========================================
+ Hits        65867    66018     +151     
- Misses      25140    25231      +91     
- Partials     5177     5185       +8

Flag	Coverage Δ
back-end	`86.13% <64.20%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...red/src/metabase/shared/parameters/parameters.cljc	`90.71% <ø> (-0.72%)`	⬇️
src/metabase/lib/temporal_bucket.cljc	`18.51% <9.52%> (-31.49%)`	⬇️
src/metabase/lib/join.cljc	`58.82% <25.80%> (-18.96%)`	⬇️
src/metabase/lib/filter.cljc	`53.93% <30.52%> (-31.98%)`	⬇️
src/metabase/lib/stage.cljc	`47.95% <47.95%> (ø)`
src/metabase/lib/aggregation.cljc	`61.81% <59.55%> (-9.62%)`	⬇️
src/metabase/lib/table.cljc	`70.00% <70.00%> (ø)`
src/metabase/lib/convert.cljc	`95.83% <75.00%> (+4.72%)`	⬆️
src/metabase/lib/metadata/protocols.cljc	`74.41% <75.60%> (+9.71%)`	⬆️
src/metabase/lib/metadata/calculation.cljc	`80.00% <80.00%> (ø)`
... and 13 more

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

snoe · 2023-03-15T19:58:42Z

src/metabase/lib/expressions.cljc

@@ -0,0 +1,143 @@
+(ns metabase.lib.expressions


I think this should be singular for consistency with lib.schema.expression

good call. I've changed it

snoe

I hope we can move column-name and display-name into defop soon, lots of boilerplate right now and it would be very easy to miss implementing one of the 3-4 multimethods that are really required for each clause.

snoe · 2023-03-15T20:17:30Z

test/metabase/lib/join_test.cljc

@@ -81,3 +100,49 @@
              (-> q1
                  (lib/join q2 (lib/= venues-category-id-metadata categories-id-metadata))
                  (dissoc :lib/metadata)))))))
+
+;;; FIXME


Is this for later?

snoe · 2023-03-15T20:18:56Z

test/metabase/lib/order_by_test.cljc

+           (describe-legacy-query-order-by query)))))
+
+(deftest ^:parallel describe-order-by-expression-reference-test
+    ;;   it("should work with expressions", () => {


looks like an unimplemented test

Oops. I'll fix this in the follow on PR I'm working on that actually flips the switch to use this in JavaScript, I need to add a few more tests there anyway

test/metabase/lib/query_test.cljc

deploysentinel · 2023-03-15T21:11:17Z

No failed tests 🎉

camsaul added 8 commits March 14, 2023 03:01

Query D E S C R I P T I O N S

e1e5284

Merge branch 'master' into MLv2-query-descriptions [ci skip]

9a57b57

WIP

4c18430

MLv2 Query description

0c52148

Test fixes

00efe2f

Merge branch 'master' into MLv2-query-descriptions

6c08f07

More robust error handling

d054858

MLv2 more shuffling things around

f6aab85

camsaul commented Mar 15, 2023

View reviewed changes

Remove unneeded impl

7fb4774

camsaul commented Mar 15, 2023

View reviewed changes

camsaul requested a review from a team March 15, 2023 18:15

camsaul commented Mar 15, 2023

View reviewed changes

snoe reviewed Mar 15, 2023

View reviewed changes

snoe approved these changes Mar 15, 2023

View reviewed changes

camsaul added 2 commits March 15, 2023 20:36

lib.expressions -> lib.expression

714732d

Address PR feedback

86f35e5

camsaul enabled auto-merge (squash) March 15, 2023 20:38

Merge commit '6154130edf' into MLv2-more-shuffling

6cab2a8

camsaul and others added 8 commits March 17, 2023 02:50

Test fixes 🔧

9c966bb

Merge branch 'master' into MLv2-more-shuffling

351d61b

Fixes post-merge

5fea21a

Merge branch 'master' into MLv2-more-shuffling

659d6c5

Fix merge conflict

c48c071

Merge branch 'master' into MLv2-more-shuffling

ab3790e

Update cypress-runner-generate-snapshots.js

2fdb54c

Update cypress-runner-run-tests.js

49c96e0

camsaul merged commit 7f15319 into master Mar 20, 2023

camsaul deleted the MLv2-more-shuffling branch March 20, 2023 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MLv2 Move metadata calculation functions into respective namespaces #29245

MLv2 Move metadata calculation functions into respective namespaces #29245

camsaul commented Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

camsaul Mar 15, 2023

codecov bot commented Mar 15, 2023 •

edited

snoe Mar 15, 2023

camsaul Mar 15, 2023

snoe left a comment

snoe Mar 15, 2023

camsaul Mar 15, 2023

snoe Mar 15, 2023

camsaul Mar 15, 2023

deploysentinel bot commented Mar 15, 2023 •

edited

		(#?(:clj p/defprotocol+ :cljs defprotocol) DatabaseMetadataProvider
		(#?(:clj p/defprotocol+ :cljs defprotocol) MetadataProvider


		(declare stage-metadata)

		(mu/defn ^:private breakout-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]


		;;; TODO -- merge this stuff into `defop` somehow.

		(defmethod lib.metadata.calculation/display-name-method :aggregation

MLv2 Move metadata calculation functions into respective namespaces #29245

MLv2 Move metadata calculation functions into respective namespaces #29245

Conversation

camsaul commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deploysentinel bot commented Mar 15, 2023 • edited

codecov bot commented Mar 15, 2023 •

edited

deploysentinel bot commented Mar 15, 2023 •

edited