Postgres dialect and PIVOT fixes surfaced by the SNPRC migration - #7993
Open
labkey-bpatel wants to merge 17 commits into
Open
labkey-bpatel wants to merge 17 commits into
labkey-bpatel wants to merge 17 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fb_sql_server_migration_2
QueryPivot.getSql() bound each pivot value using the constant's own JdbcType, but wrapConstant() never produces QDate/QTimestamp -- a date or timestamp pivot column arrives here as a QString, so the parameter was bound as VARCHAR and Postgres rejected the comparison with "operator does not exist: timestamp without time zone = character varying". SQL Server converts implicitly, so only Postgres was affected. Prefer the pivot column's own type and convert the value to it, falling back to the constant's type and value when that conversion doesn't hold, since the IN (SELECT ...) form can supply values from an expression other than the pivot column. Where the two types already match -- every string pivot -- the emitted SQL and bound value are unchanged.
_makePivotedAggColumn() returns null whenever parse errors are present, so QueryPivot.getAllColumns() silently omitted every <value>::<agg> column. The TableInfo's column set is built from getAllColumns() through initializeColumns(), and that path never reaches getSql(), so a failure such as "Pivot query unauthorized" produced a successfully-constructed table missing all of its pivoted columns with no error raised anywhere -- the schema browser, getQueryDetails and the custom view field picker all show the query as simply having no pivoted output. Throw the underlying parse error the way getSql() and getColMembers() already do, and discard the partially-built _columns map first so a later call re-derives it rather than serving the short list from cache. Only fires when a column was actually dropped, so the complete-column path is unaffected.
…fb_sql_server_migration_2
4 tasks
labkey-bpatel
requested review from
labkey-adam,
labkey-martyp and
labkey-matthewb
September 2, 2026 16:34
labkey-adam
approved these changes
Sep 3, 2026
labkey-adam
left a comment
labkey-adam
left a comment
Contributor
There was a problem hiding this comment.
Any tests for these changes?
Covers the Postgres dialect override that emits US-style week numbers. The dates are chosen to exercise both divergent rules and the year boundary; a mid-year sample in a Mon-Thu year passes with the bug fully present.
3 tasks
Contributor
Author
week() is a passthrough, so overriding it on Postgres moved WeekOfYear for every existing caller, including the shared EHR and LDK date range lookups. The two new functions are defined by LabKey and return the same number on either dialect, so callers pick a numbering instead of inheriting the driver's.
labkey-matthewb
self-requested a review
September 15, 2026 17:10
labkey-matthewb
approved these changes
Sep 15, 2026
This is diagnostics rather than a correctness fix, and it changes behavior every TableInfo runs through, so it moves to its own PR along with the AbstractTableInfo init-guard fix it needs to be safe.
isnumeric() began as a SQL Server passthrough returning 1/0, so callers compare it with = 1. PostgreSQL returns 1/0 again and SQL Server resolves to that passthrough, restoring the one spelling that works on both.
labkey-bpatel
added a commit
to LabKey/snprcEHRModules
that referenced
this pull request
Sep 15, 2026
isnumeric() returns 1/0, not a boolean. The comparison was dropped to match a platform change that is reverted in LabKey/platform#7993.
The formula needs the argument twice, so it was spelled out twice, duplicating the SQL, its bound parameters and any side effects. A VALUES subquery names it instead.
The value was bound as a typed parameter, which needed its JdbcType patched from the pivot column and spent one bind per value per aggregate. Letting the constant emit itself quotes through the dialect instead, with no parameters and no conversion.
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
3 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
Migrating the SNPRC EHR deployment from SQL Server to Postgres called for a handful of adjustments in shared platform code so that LabKey SQL produces matching results on both databases. Where the two backends legitimately differ, nothing shared is redefined:
week()still defers to the driver, with its two possible numbering schemes now available as explicit functions, andisnumeric()keeps its 1/0 contract. The PIVOT changes concern how a pivot value reaches the generated SQL rather than either database in particular. These came up through SNPRC's queries rather than through general use, and each change is a no-op for queries that already produce matching results.Related Pull Requests
Changes
weekiso()(ISO 8601, 1 to 53) andweekus()(US, 1 to 54), defined by LabKey on both dialects instead of by whichever expression a driver picks for{fn week(x)}.week()itself is untouched: it has been a passthrough since 2010, and the sharedehr_lookups.dateRange,ehr_lookups.next30Daysandldk.dateRangelookups all read it, so redefining it would have silently shiftedWeekOfYearfor every existing Postgres deployment. SNPRC overrides its own copies of those three lookups to callweekus(). The SQL Server implementations live inpremiumModules.isnumeric()returns 1/0 on both databases — SQL Server resolves it to themssqlMethodspassthrough, Postgres to a regex-basedCASE, so callers compare it with= 1. Matching changes inpremiumModulesandsnprcEHRModules._makePivotedAggColumn()returns null whenever parse errors are present, and that null was going into the column map.