Port query description logic to MLv2 #29200
Conversation
camsaul
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #29200 +/- ##
==========================================
+ Coverage 68.46% 68.51% +0.04%
==========================================
Files 2788 2790 +2
Lines 96474 96579 +105
Branches 12297 12316 +19
==========================================
+ Hits 66054 66167 +113
+ Misses 25233 25205 -28
- Partials 5187 5207 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 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. |
|
No failed tests 🎉 |
| let metadata = | ||
| tableMetadata?.id != null | ||
| ? assocIn( | ||
| this._metadata, | ||
| ["tables", String(tableMetadata.id)], | ||
| tableMetadata, | ||
| ) | ||
| : this._metadata; |
There was a problem hiding this comment.
I'm not sure why we need to modify the metadata here and below now?
There was a problem hiding this comment.
MLv2 currently wants queries to be initialized with complete metadata like this._metadata, not just metadata for a single table. So I'm just merging the metadata for the individual table into this._metadata.
I suppose we could rework things so we can initialize a query with just the metadata for a single table but it seems like query descriptions would break if you happened to be filtering on a field from a different table, so it didn't make a ton of sense to relax that rule honestly
There was a problem hiding this comment.
@camsaul I see, thank you. What if we rework the caller so it actually provides the full Metadata instance instead?
I can open a PR doing this against your branch or make another PR once this one is merged
| _getMLv2Query(metadata = this._metadata) { | ||
| return MLv2.query(this.databaseId(), metadata, this.datasetQuery()); | ||
| } |
There was a problem hiding this comment.
Would it make sense to make the method private?
| @@ -1605,47 +1605,6 @@ describe("Question", () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("Question._getOrderByDescription", () => { | |||
There was a problem hiding this comment.
these test a specific part of getQueryDescription, tests are in cljs now
Implements #28882
Replaces a solid 300 Loc from JS metabase lib with Cljs versions. That's a little over 1% of metabase-lib, so if we just open 99 more PRs like this we'll be done. Nice
This ports the version of
generateQueryDescriptioninQuestion.ts, rather than the version inqueries/utils/description.jswhich is pretty much an exact copy, but somewhat harder to port since it's used without a full query. The version inQuestion.tsis used more extensively anyway. I'll try to port that over as well as a follow-on.This PR seems bigger than it is, but that's because I shuffled around a lot of multimethods used for metadata calculation and moved their implementations to live with the other functions related to them e.g. the
display-namemethod for aggregations lives inmetabase.lib.aggregationsnow instead of inmetabase.lib.metadata.calculate.names. So like 90% of this PR is just moving stuff around so it's easier to deal with going forward.I'm also working on porting over a ton of other stuff, e.g.
Aggregation.displayName(), but I'll open a follow-on PR for that.