-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use FOLIO statistical codes to determine if this should be a database… #981
Conversation
6659efb
to
7b001e4
Compare
7b001e4
to
a65a224
Compare
ON item.holdingsrecordid = hr.id | ||
ON item.holdingsrecordid = hr.id | ||
-- Instance's statistical code | ||
LEFT JOIN LATERAL jsonb_array_elements_text(vi.jsonb -> 'statisticalCodeIds') uuid_stats_code(id) ON TRUE |
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.
Did you do any performance testing with this approach? It looks like it triggers a relatively high cost function scan, but maybe it's no worse than the rest of this nasty query?
If it's a problem, I guess we'd revert this part of the query and resolve the statistical code names some other way?
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 created #987 so that we have some reproducible way to evaluate the performance of query changes.
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 indeed causes the test ./spec/lib/traject/readers/folio_postgres_reader_spec.rb
to fail. I will think about another approach.
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.
Hm. I was thinking the performance was in the margin of error (although I'm consistently getting timeouts everywhere now).. I was seeing ~45s for main
and ~60s for this branch, but smaller time-slices were pretty much identical.
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.
@cbeer I added a second commit that has the same effect, but relies on a second query rather than adding joins.
1ff3c6f
to
5a79804
Compare
5a79804
to
1de5446
Compare
30433dc
to
960e4d6
Compare
… type
Fixes #948