-
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
Fix excessive DB calls on reference guide #6638
Conversation
src/metabase/models/field.clj
Outdated
(filter i/can-read? (db/select Field :id [:in target-field-ids]))))] | ||
(filter i/can-read? (-> Field | ||
(db/select :id [:in target-field-ids]) | ||
(hydrate :table)))))] |
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.
extra whitespace? Also personal preference but
(-> (db/select Field :id [:in target-field-ids])
(hydrate :table))
is the way we do it everywhere else and personally I think that would be a little more readable here :)
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.
one other thought: we should probably go ahead and remove the :table
after we use it for can-read?
purposes. Otherwise it will get returned as part of the API response for every Field, which could potentially dramatically increase the size of the response
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.
😆 I added that space because I figured that's what you'd do. I'll remove it.
👍 on removing table after as well
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.
I’ve never seen the two spaces like that before. Is that a convention for threading macros? I see how it could make sense
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.
overall LGTM. A few minor thoughts noted
probably not a bad idea to do a quick audit of places where we call |
@camsaul I briefly looked through the code for more instances of that and didn't see any. I'll double check a little more carefully though to be sure. |
Before this commit, via the `/database/:id/metadata`, we make a DB call for each field's target table. On databases with a larger number of these values, it can cause many database calls. This will fetch them via batch hydration first, which will significantly reduce the number of calls.
c5d0c14
to
d86e7d9
Compare
Before this commit, via the
/database/:id/metadata
, we make a DBcall for each field's target table. On databases with a larger number
of these values, it can cause many database calls. This will fetch
them via batch hydration first, which will significantly reduce the
number of calls.