Update holdings_ext.sql

[VandanaShah-Cornell]

/*Changes:

1. Fixed to bring in the correct data for the 'updated_by_user_id' field; it was bringing in data from the created by userid rather than the updated_by_user_id.
2. The holdings_supress field needed to be cast as Boolean.
3. The json extract for ill_policy_id needed to be cast as UUID.
4. Cast the date fields as "timestamptz" (changed from text)
5. Cast the discovery_suppress field as Boolean (changed from text)
6. Cast the extract of updatedByUserId as UUID (changed from text) */

[VandanaShah-Cornell]

This PR closes issue #856

[trapido]

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:

• PR Title and Description are accurate and thorough
• PR is based on a new branch (not main)
• PR scope is not overly broad
• Query runs without errors
• Query output is correct
• Query logic is clear and well documented
• Query is readable and properly indented
• Table and column names are in all-lowercase
• Quotation marks are used only where necessary
• JSON extraction is in standard form, for example:
  LDP: t #>> '{f1,f2,f3}' [for compatibility across LDP 1 & 2]
  Metadb: jsonb_extract_path_text(t, f1, f2, f3)

Derived tables:

• First line is "--metadb:table" directive, followed by blank line
• User documentation in comment lines, followed by blank line
• File name is listed in runlist.txt after dependencies

[VandanaShah-Cornell]

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.

[VandanaShah-Cornell]

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: * [X] PR Title and Description are accurate and thorough * [X] PR is based on a new branch (not main) * [X] PR scope is not overly broad * [ ] Query runs without errors * [ ] Query output is correct * [ ] Query logic is clear and well documented * [X] Query is readable and properly indented * [X] Table and column names are in all-lowercase * [X] Quotation marks are used only where necessary * [X] JSON extraction is in standard form, for example: LDP: t #>> '{f1,f2,f3}' [for compatibility across LDP 1 & 2] Metadb: jsonb_extract_path_text(t, f1, f2, f3) Derived tables: * [X] First line is "--metadb:table" directive, followed by blank line * [X] User documentation in comment lines, followed by blank line * [X] File name is listed in runlist.txt after dependencies - Reply to this email directly, view it on GitHub<#857 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMK23BQ4E7R6TGJ7VURUC23ZJWUPFAVCNFSM6AAAAABJIKRMCWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBYG4YTONJZHE>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>

[VandanaShah-Cornell]

Re: Using both holdings_record__t and holdings_record tables : most of the fields are already extracted from the jsonb and are in the holdings_record__t 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.

[trapido]

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.