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

Count Field usage #39671

Merged
merged 49 commits into from
Apr 18, 2024
Merged

Count Field usage #39671

merged 49 commits into from
Apr 18, 2024

Conversation

qnkhuat
Copy link
Contributor

@qnkhuat qnkhuat commented Mar 6, 2024 •
edited
Loading

Closes #40494
Milestone 2 of #38229

@qnkhuat qnkhuat requested a review from camsaul as a code owner March 6, 2024 02:45
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Mar 6, 2024
@replay-io replay.io
Copy link

replay-io bot commented Mar 6, 2024 •
edited
Loading

Status Complete ↗︎
Commit 94afdca
Results
⚠️ 13 Flaky
✅ 2396 Passed

qnkhuat and others added 9 commits March 22, 2024 11:37
@qnkhuat
Merge branch 'master' into count-field-usage
b04999a
@qnkhuat
reorder migration ids
cd1d3c7
@qnkhuat
Merge branch 'master' into count-field-usage
98160d9
@qnkhuat
field-count POC
e190650
@qnkhuat
better naming
825b221
@qnkhuat
Merge branch 'master' into count-field-usage
e59e320
@rafpaf @qnkhuat
Restore Models in Browse data (#40633)
42d5aaf 
* Revert "Remove Models from Browse data (#40108)"

This reverts commit 45a2e86.

* Make rtl friendly

* Add 'no description' for blank-description models

* Fix vertical position of collection header toggle

* Move FixedSizeIcon into metabase/ui and increase height of empty state

* Move FixedSizeIcon back into BrowseModels.styled.tsx

* Localize 'No description'

* Add comment

* Fix type issue
@nemanjaglumac @qnkhuat
Trigger a query limit update on blur while in notebook mode (#40630)
5a856cd 
* Trigger the query limit change on blur

* Use named export

* Add a unit test

* Fix E2E tests

* Fix unit tests

* Additional E2E test fix

* Address review comments
@oisincoveney @qnkhuat
Convert headings.module.css to CSS modules (#40570)
416e289
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label Mar 27, 2024
@github-actions GitHub Actions
Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff 5fb43f2...416e289.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/visualizations/Table.tsx

@qnkhuat
- Break breakout_binning to 3 granualar columns:
ea4aff0 
  breakout_binning_num_bins, bin_width, strategy
- Remove filter_args as we might need a better way to store those args
- no more multimethod, defn all the way
@qnkhuat
Copy link
Contributor Author

qnkhuat commented Apr 4, 2024

okay so we(w/ Cal) had a huddle and decided that we don't want to store any json in this table of ease of querying in the future. No one like looking into parsing breakout_binning to get :strategy in SQL.

instead we'll now have 3 more columns: breakout_binning_strategy, breakout_binning_bin_width, breakout_binning_num_bin.

Also removed filter_args because of this reason. We'll revisit storing this in a different shape when we need it. One idea is to store one arg per row.

calherries
calherries approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@calherries calherries left a comment

Choose a reason for hiding this comment

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

Great, looks good from my perspective. I'd still like someone from QPD to review the MLv2 usage in src/metabase/models/field_usage.clj, and the changes in metabase.lib, and metabase.query_processor though

@qnkhuat qnkhuat requested a review from a team April 4, 2024 11:50
bshepherdson
bshepherdson requested changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@bshepherdson bshepherdson left a comment

Choose a reason for hiding this comment

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

A few miscellany, but the note about misusing the QP middleware is important. Please fix that.

src/metabase/lib/util.cljc Outdated Show resolved Hide resolved
src/metabase/models/field_usage.clj Show resolved Hide resolved
src/metabase/models/field_usage.clj Show resolved Hide resolved
src/metabase/query_processor/middleware/process_userland_query.clj Outdated
@@ -141,6 +154,12 @@
:result_rows 0
:start_time_millis (System/currentTimeMillis)})

(defn- query->field-usages
[query]
(->> (normalize/normalize-preprocessing-middleware query)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is misusing the middleware. It's not a helper function intended to be called like this. The middleware runs in a defined order, see metabase.query-processor.preprocess/middleware for the big list.

In particular, you'll see that normalize/normalize-preprocessing-middleware is the first step, and fetch-source-query/resolve-source-cards is near the top. So you shouldn't have to call either of these - the query will be fully preprocessed already.

test/metabase/query_processor/middleware/process_userland_query_test.clj Outdated Show resolved Hide resolved
@qnkhuat
Copy link
Contributor Author

qnkhuat commented Apr 11, 2024

@bshepherdson I've updated so we don't process the query in process-userland-query middleware, instead we get the passed the processed query through metadata and then dissoc it before returning.

Please help reviews!

piranha
piranha reviewed Apr 11, 2024
View reviewed changes
src/metabase/query_processor/middleware/process_userland_query.clj Outdated
(let [preprocessed-query (:preprocessed_query metadata)
;; we only need the preprocessed query to find field usages, so make sure we don't return it
result (rff (dissoc metadata :preprocessed_query))
field-usages (field-usage/pmbql->field-usages
Copy link
Contributor

Choose a reason for hiding this comment

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

This seem to be happening synchronously during query execution, I wonder if there is any sense into passing it somewhere to execute async? Especially if we add parsing SQL in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym by parsing SQL?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well right now it get field usages from mbql queries, right? But not from SQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, only mbql

@qnkhuat qnkhuat requested a review from a team April 11, 2024 06:06
@qnkhuat qnkhuat added no-backport Do not backport this PR to any branch and removed visual Run Percy visual testing labels Apr 11, 2024
@qnkhuat qnkhuat enabled auto-merge (squash) April 17, 2024 02:35
@qnkhuat qnkhuat mentioned this pull request Apr 17, 2024
Closed
@qnkhuat qnkhuat requested a review from noahmoss as a code owner April 18, 2024 03:04
@qnkhuat qnkhuat merged commit d1a7cec into master Apr 18, 2024
105 checks passed
@qnkhuat qnkhuat deleted the count-field-usage branch April 18, 2024 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piranha piranha piranha left review comments

@bshepherdson bshepherdson bshepherdson approved these changes

@calherries calherries calherries approved these changes

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

@crisptrutski crisptrutski Awaiting requested review from crisptrutski

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

Assignees

@qnkhuat qnkhuat

Labels
no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Record field usage during query execution
8 participants
@qnkhuat @calherries @piranha @bshepherdson @crisptrutski @rafpaf @nemanjaglumac @oisincoveney