Closed VandanaShah-Cornell closed 1 week ago
This PR closes issue #856
My apologies for these errors, looks like I was testing the query without creating a table, and then sent the version I was mucking around with as a PR, instead of the cleaned version.
Hi Irina,
I am so sorry about this, looks like I was testing the query without creating a table, and then sent version I was mucking around with as a PR, instead of the cleaned version.
-Vandana
From: trapido @.> Sent: Friday, June 28, 2024 2:38 PM To: folio-org/folio-analytics @.> Cc: Vandana Suresh Shah @.>; Author @.> Subject: Re: [folio-org/folio-analytics] Update holdings_ext.sql (PR #857)
@trapido requested changes on this pull request.
There are a couple of problems: 1.The query won't run because the CTE is not properly terminated with ")" and the main query is missing a select statement. The easiest way to fix this might be to remove line 11, "WITH holdings AS ( ". 2.I think that joining holdings_record__t and holdings_record tables might be redundant: it should be possible to extract all of the data out of holdings_record table.
All queries:
Derived tables:
- Reply to this email directly, view it on GitHubhttps://github.com/folio-org/folio-analytics/pull/857#pullrequestreview-2148717599, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AMK23BQ4E7R6TGJ7VURUC23ZJWUPFAVCNFSM6AAAAABJIKRMCWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBYG4YTONJZHE. You are receiving this because you authored the thread.Message ID: @.**@.>>
Re: Using both holdings_recordt and holdings_record tables : most of the fields are already extracted from the jsonb and are in the holdings_recordt table, except for three fields that needed to be extracted from the holdings_record table. It is easier to use the transformed table, is there any reason to not do this? Seems like extra work to extract fields that are already in transformed tables.
Thank you very much, Vandana! Everything looks good. Just one minor thing: should the metadb dirctive read "--metadb:table holdings_ext" instead of "--Metadb: holdings_ext"? Regarding using holdings_record__t and holdings_record tables in the same query, there is nothing wrong with this, I just remember Nassib mentioning that this is not efficient because we are essentially joining two copies of the same data.
/*Changes: