SQL Server Migration 2 - #990
Open
labkey-bpatel wants to merge 42 commits into
Open
Conversation
… and fix upgrade script. Fix query errors during Folder import.
…fb_sql_server_migration_1
…fb_sql_server_migration_1
…fb_sql_server_migration_1
…ries portable to Postgres (also fixes test failures)
…fb_sql_server_migration_1
…fb_sql_server_migration_1
…abKey/snprcEHRModules into 26.3_fb_sql_server_migration_1
SNPRC_schedulerTest now enables SNPRC_EHR and routes through the helper for SND/EHR domain init, SND category and package seeding, animal group tables, and the Core Facilities/Genetics subfolder plus assay design uploads; moves populateEHRTables() before importStudy() to match SNPRC_EHRTest's populateHardTableRecords-before-import ordering; and turns on import-time query validation. Guard BeginPage.beginAt against a missing modal-backdrop element so it stops printing a spurious NoSuchElementException stack trace on every navigation.
…fb_sql_server_migration_2
…UIDs Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The existing snprc_ehr schema handler was leaving GUID FK columns on study datasets (e.g. assay_labworkResults.serviceTestId) uppercase after migration, breaking their FKs to lowercase-canonicalized entityid targets like labwork_panels.ObjectId on Postgres. Extend registerMigrationHandlers with schema contributors for studydataset, ehr, and ehr_lookups so GuidMapperColumn covers those tables too. Also add LookupSetNameCaseMapperColumn for ehr_lookups.lookups: SS's case-insensitive collation had let set_name values drift out of case with lookup_sets.setname (camelCase 'accessionCode' vs PascalCase 'AccessionCode'), which broke the framework-generated filter queries on Postgres. COALESCE-JOIN against lookup_sets at migration time normalizes to the canonical case. Dedup key switched from table.getSelectName() to composite '<schema>.<table>' so bare-name collisions across the new schemas can't silently skip a table. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DISTINCT + GROUP BY on the free-text admitcomplaint column returned different counts on Postgres than SQL Server because SS's case-insensitive collation collapsed variants like 'Trauma' vs 'trauma' or trailing-whitespace copies into single groups while PG kept them apart, dropping some groups below the HAVING count > 10 threshold. Wrap admitcomplaint with UPPER(LTRIM(RTRIM(...))) in both SELECT and GROUP BY so both databases return the same set (45 rows). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Source has case-variant animal IDs (e.g. 4x0133 vs 4X0133) that SS's case-insensitive collation grouped together in the outer GROUP BY; Postgres is case- and whitespace-sensitive so they split into separate output rows, producing 4512 rows on PG vs 4511 on SS. Wrap bn.Id with UPPER(LTRIM(RTRIM(...))) in the SELECT, GROUP BY, and ORDER BY so both databases return the same 4511 rows. The Behavior and BehaviorDescription columns don't have this variance, so those stay unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two overlapping causes of the +158 row discrepancy PG vs SS. First, case/whitespace variants on Id, Haplotype, Ocid, and DataFileSource were splitting groups that SS's collation collapsed — wrap the four columns with UPPER(LTRIM(RTRIM(...))) in the inner subquery so both databases see the same canonical form at GROUP BY and PIVOT time. Second, SS's default cast(datetime as varchar) is minute-precision while PG's preserves microseconds, so identical-looking timestamps stayed distinct on PG and split groups. Aggregate b.modified via MAX() in the outer SELECT and remove it from GROUP BY; grouping is no longer sensitive to sub-second drift, and the output still shows the latest modification per pivot row. Add a DateTime formatString on modified in the query metadata for consistent display. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lt.ServiceId.ServiceName has case/whitespace variants in the source data (e.g. 'Ova & Parasites' vs 'OVA & PARASITES'). SS's case-insensitive collation matched both against the NOT IN list; Postgres is case- and whitespace-sensitive so 2059 variants slipped past the exclusion, giving 2077 rows on PG vs the intended 18. Wrap the column with UPPER(RTRIM(LTRIM(...))) so both databases produce 18. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The report's WHERE clauses filter on b.qualresult, b.serviceTestId.testName, and b.serviceTestid.ServiceId.Dataset. SS's case-insensitive collation matched them regardless of source-data casing; Postgres is case-sensitive and drops rows where the case doesn't line up exactly. Wrap each filter column and the CASE b.qualresult expression with UPPER(LTRIM(RTRIM(...))). Also wrap b.id (animal ID) in the inner SELECTs so case-variant animal IDs collapse in the outer GROUP BY the same way SS does. Same fix applied to both the study and snprc_ehr copies of this query since they carry the same body. Correctness against the base data still depends on the migration handler widening that lands the missing assay_labworkResults rows on PG; this commit only fixes the query-side case-sensitivity concerns. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fb_sql_server_migration_2
safeAppendTimeFromDate() and safeAppendDateAndTime() read the column with rs.getString() and parsed it against a fixed "yyyy-MM-dd HH:mm:ss[.f]" pattern. The string form is driver-specific, so that pattern has to track every driver's rendering -- it was recently widened to make fractional seconds optional, and a timestamptz column through pgjdbc would still fail on the trailing offset. Read the value as a Timestamp and format from LocalDateTime instead, which removes the dependency on the driver's string form entirely. Also hoists the output DateTimeFormatter into a constant; it was being reallocated on every rendered row.
The SNPRC source data has case/whitespace variants of the same unit (e.g. 'mg' vs 'MG'). SQL Server's case-insensitive collation collapsed them on DISTINCT; Postgres returns them as separate rows. Canonicalize with UPPER(LTRIM(RTRIM(...))) here rather than in the shared ehr module: lab_test_units is an FK target, not just a display list -- its query.xml declares units as the key field, and it is referenced as <fkTable>lab_test_units</fkTable> by ehr's study/chemistryResults, study/hematologyResults and study/urinalysisResults, and directly by WNPRC_EHR's datasets_metadata.xml. Uppercasing changes the set of key values the lookup offers, so it should not be imposed on the other centers.
…fb_sql_server_migration_2 # Conflicts: # snprc_ehr/src/org/labkey/snprc_ehr/SNPRC_EHRModule.java # snprc_ehr/src/org/labkey/snprc_ehr/history/DefaultTreatmentOrdersDataSource.java # snprc_ehr/src/org/labkey/snprc_ehr/history/LabworkDataSource.java
Postgres compares text case-sensitively where SQL Server's default collation does not, so several SNPRC EHR queries that were correct by accident on SQL Server return wrong results after the migration. Normalize panel, test and result names with UPPER(RTRIM(LTRIM(...))) in the labwork pivot queries for Culture, Misc Tests and Surveillance. The PIVOT IN list is built from snprc_ehr.labwork_panels while the pivoted rows come from study.labworkResults and the HL7 OBR/OBX tables; on SQL Server those matched regardless of case and padding, but on Postgres they have to match exactly or the pivot columns come back empty. Both sides are now normalized the same way. Correct the 'OKAY' literal to 'Okay' in demographicsMostRecentPhysicalDate and demographicsMostRecentTBDate. study.overdueTb filters on PhyStatus/TbStatus <> 'Okay', so on Postgres the mis-cased literal put infant baboons on the overdue reports; SQL Server's case-insensitive comparison hid the typo. Point the admin 'Enter Data' link on the SNPRC begin page at ehr-enterData.view instead of the legacy/Ext3 ehr-dataEntry.view
DeletedLookups and DeletedLookupSets pull objectid out of the stored audit record by searching oldrecordmap for the text 'objectid=' and taking the following 36 characters. The literal was lowercase and locate() is case-sensitive on Postgres, so the search failed on every row, while SQL Server's case-insensitive collation matched regardless of how the key was stored. The failure was silent rather than loud. With locate() returning 0 the substring started at a fixed offset instead of stopping, so all 130 rows came back as 36-character fragments of unrelated text filling a GUID-shaped column, e.g. 'tId=2177&value=ddfsdfd&displayable=t' in place of '02c68113-c5a4-103b-b46b-9caba13b78b2'. Anything joining on objectid matched nothing, with no error and no null to signal it. Search a lower-cased copy of oldrecordmap so any casing is found. lower() preserves length, so the offset arithmetic stays valid for substring() on the original and the extracted value keeps its stored casing. Hard-coding the casing instead would not be safe: the audit map's keys come from the row map the update service supplied (see AbstractAuditTypeProvider.encodeForDataMap), so they follow the calling code's naming rather than the declared column, and these are historical rows written by several code paths. Verified on DeletedLookups against a Postgres capture: objectid now returns GUIDs matching SQL Server. DeletedLookupSets carries the identical change but was not separately verified.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
Part 2 of the SQL Server to PostgreSQL migration, continuing from #955. Where #955 covered SQL syntax portability, this PR addresses collation: SQL Server compares text case- and whitespace-insensitively, so source data carrying variants — animal IDs as
4x0133and4X0133, units asmgandMG, lookup values differing only in capitalisation — was being collapsed silently atDISTINCT,GROUP BY,UNIONandPIVOTtime. PostgreSQL compares exactly. Each affected site is now canonicalised in the query, or normalised once at migration time where that repairs more consumers at once. Results were reconciled query by query against the SQL Server source; one report's value legitimately changes, noted below.Related Pull Requests
Changes
Grouped by why the two databases disagreed. Each file carries a comment explaining its own case.
Collation — SQL Server compares text case- and whitespace-insensitively and these sites relied on it, at
GROUP BY,DISTINCT,UNIONdedup,PIVOTvalue lists and string filters.MhcDataPivot,AdmitComplaintList,BehaviorAbnormalSummary,Paraasitology,proceduresPerYear, bothReportTcruziSummaryAllcopies, the Parasitology / culture / misc / surveillance pivots and their inner queries,demographicsMostRecentTBDateanddemographicsMostRecentPhysicalDate(an'OKAY'branch among'Okay'siblings put 30 infant baboons on the overdue report),DeletedLookups,DeletedLookupSets,R24WeightsDeletedSrc,DemographicsHL7DeleteSource(the last two returned zero rows on Postgres), and migration-time normalisation inSNPRC_EHRModulefor GUID columns and two case-sensitive natural-key FKs.Type coercion — the engines convert types differently, or refuse to.
animalDemographics(CAST(x AS DECIMAL)is(18,0)on SQL Server, unspecified scale on Postgres),demographicsMostRecentWeight(rounding routed throughNUMERICrather thandouble), the two clinical-history data sources (timestamps read viagetTimestamp()instead of parsing the driver's string rendering), and the PostgreSQL bootstrap (HL7_IMPORT_LOG.TIDbecomes aBIGINTidentity — Postgres identity columns cannot beNUMERIC).Dialect semantics — the same named function implemented differently.
validateFinalWeights, an SNPRC override:timestampdiffcounts calendar-date crossings on SQL Server and elapsed 24-hour periods on Postgres, which moved 31 animals across a>= 7filter.Neither — bugs the comparison exposed.
culturePivotInner's twoUNIONlegs listed their columns in opposite order, so panel and test name were transposed for every HL7-sourced row;colonyUsageQuerypivoted on a gender label that resolves toMaleon Postgres andMon SQL Server, and referenced pivot columns with casing its subquery alias did not match (quoted identifiers are case-sensitive on Postgres);AnimalsHierarchyControllerfiltersvisiblewith a boolean rather than1;SNPRC_schedulerContainerListenerruns its cleanup in a transaction; a missing statement terminator inis_numeric.Scoped deliberately.
lab_test_unitsis an SNPRC override rather than a change to the sharedehrquery, which is an FK target consumed byWNPRC_EHR— uppercasing its values should not reach other centres.demographicsMostRecentWeightandvalidateFinalWeightsare overrides for the same reason.One report value changes.
ReportTcruziSummaryAllnow keys itsUNIONdedup onTestIdrather than the test name, becauselabwork_panelsholds 850 asT. CRUZI ABand 965 asT. Cruzi AB— two codes distinguishable only by capitalisation. Animal 28215 goes 5 → 6 and the total 257 → 258; Postgres's existing values are unchanged. Assumes the two codes are genuinely different assays — still an open question for SNPRC.Tests. Shared setup extracted into
SnprcSetupHelper;SNPRC_EHRTestandSNPRC_schedulerTestupdated for PostgreSQL; sample-dataStatusvalues normalised.Tasks 📍