Disable LabKey caching of ResultSet by default by labkey-jeckels · Pull Request #7711 · LabKey/platform

labkey-jeckels · 2026-06-02T00:12:59Z

Rationale

We don't want to cache more than we need to. We just disabled Postgres JDBC caching for QueryService.getSelectBuilder() scenarios. Most scenarios shouldn't need a LabKey cache either.

Changes

Stop using CachedResultSet by default
Opt in to caching when needed
Tests for result set scrollability
Improve generics
Migrate remaining QueryService.select() and QueryService.selector() methods to QueryService.getSelectBuilder()

Tasks 📍

Claude Code Review
~~Manual Testing~~
Test Automation

labkey-jeckels · 2026-06-02T00:16:32Z

I queued MS2 and Panorama suites as they may reverse ResultSets as part of rendering their nested grids.

https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_ModuleSuites_Ms2Postgres/4022367

https://teamcity.labkey.org/buildConfiguration/LabKey_Trunk_External_Panorama_PanoramaAPostgres/4022369

https://teamcity.labkey.org/buildConfiguration/LabKey_Trunk_External_Panorama_PanoramaBPostgres/4022371

…efault

QueryHelper funnels every select through SelectBuilder.select(), whose default flipped to an uncached, streaming result set on this branch. Many QueryHelper callers (15 files across wnprc-modules, ehrModules, OConnorLabModules, and premiumModules) rely on the cached result set's full API — particularly getRowMap(), which a streaming result set does not support. In wnprc-modules' SimpleQuery the resulting UnsupportedOperationException is swallowed and surfaces as an empty result, causing WNPRC_EHRTest's consistent "Account acct103 not found in aliases table" setup failure on this branch. Request a cached result set explicitly to preserve the helper's longstanding semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…efault

labkey-adam

Some comments to consider, but no further review required

labkey-adam · 2026-06-04T17:56:48Z

            tableMap.put("__DATA", assayDataTable);

-            try (ResultSet rs = QueryService.get().select(schema, sqlf.getSQL(), tableMap, false, true))
+            try (ResultSet rs = QueryService.get().getSelectBuilder(schema, sqlf.getSQL(), false, tableMap).select(true))


Don't we want getSelectBuilder() to take a SQLFragment?

@labkey-matthewb and I discussed this. Ultimately, we want it to take a LabKeySQLFragment or similar and keep SQLFragment for raw DB SQL.

labkey-adam · 2026-06-04T17:58:17Z

                ArrayList<String> ids = new ArrayList<>();
                while (rs.next())
                    ids.add(rs.getString(1));
                return ids;


Ideally, we'd convert this all to .buildSqlSelector().getArrayList(String.class); Maybe that's for another day.

Yes, I don't want to figure out how to test this right now.

labkey-adam · 2026-06-04T18:05:05Z

        default Results select()
        {
-            return select(true);
+            return select(false);


Maybe too late for this, but it would be nice to replace the various boolean parameters with enums that elucidate the behaviors.

Good idea for next time.

labkey-adam · 2026-06-04T18:05:57Z

-        return select.select();
+        // select(true): legacy callers (especially EHR modules) rely on the cached result set's full API,
+        // e.g. getRowMap(), which a streaming result set does not support
+        return select.select(true);


labkey-adam · 2026-06-04T18:12:31Z

            dcMySampleParent = mySampleParent.getRenderer();

            assertTrue(results.next());
            ctx.setRow(results.getRowMap());


Calling getRowMap() requires a cached Results, right? Should we document that method as such?

JavaDoc added.

labkey-adam · 2026-06-04T18:14:43Z

-        }
+                if (!_container.equals(that._container)) return false;
+                return _schema.equals(that._schema);
+            }


Isn't the default equals() essentially this?

Seems odd to remove hashCode() but not equals()

Yes. Removed. I told IntelliJ to automatically apply some auto-refactors on save and it's surprisingly aggressive at converting some classes to records.

labkey-adam · 2026-06-04T18:16:50Z

-                    return false;
-
-                return true;
+                if (obj instanceof SessionQuery(String sql, String metadata))


Whoa, what is this strange syntax? Is that strictly a record thing?

Again, default equals() won't do here?

Same thing. Simplified.

…efault

#### Rationale As we work to reduce OutOfMemoryError risk, we need to have places that rely on caching Results behavior opt-in. We want to consolidate on using a single builder approach for these selects. #### Related Pull Requests * LabKey/platform#7711 #### Changes * Migrate to `QueryService.getSelectBuilder()`

#### Rationale As we work to reduce OutOfMemoryError risk, we need to have places that rely on caching Results behavior opt-in. Here, it's so that we can ask the size of the result set before iterating it. #### Related Pull Requests * LabKey/platform#7711 #### Changes * Opt-in to a `CachedResultSet`

Disable LabKey caching of ResultSet by default

5714d8d

labkey-jeckels requested a review from labkey-adam June 2, 2026 00:12

labkey-jeckels self-assigned this Jun 2, 2026

Disable LabKey caching of ResultSet by default

26acdd3

This was referenced Jun 2, 2026

Disable LabKey caching of ResultSet by default LabKey/targetedms#1222

Merged

Disable LabKey caching of ResultSet by default LabKey/commonAssays#1020

Merged

labkey-jeckels added 3 commits June 2, 2026 18:22

Disable LabKey caching of ResultSet by default

18dfa27

Merge remote-tracking branch 'origin/develop' into fb_disableCacheByD…

18d88e4

…efault

Merge remote-tracking branch 'origin/develop' into fb_disableCacheByD…

14df226

…efault

labkey-jeckels mentioned this pull request Jun 3, 2026

Disable LabKey caching of ResultSet by default LabKey/wnprc-modules#967

Merged

labkey-jeckels and others added 2 commits June 3, 2026 20:50

Merge remote-tracking branch 'origin/develop' into fb_disableCacheByD…

6b38f6c

…efault

labkey-adam approved these changes Jun 4, 2026

View reviewed changes

labkey-jeckels added 2 commits June 4, 2026 17:20

Merge remote-tracking branch 'origin/develop' into fb_disableCacheByD…

23bee97

…efault

Code review suggestions

e4fef18

labkey-jeckels merged commit b47a842 into develop Jun 6, 2026
11 checks passed

labkey-jeckels deleted the fb_disableCacheByDefault branch June 6, 2026 16:05

Conversation

labkey-jeckels commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Changes

Tasks 📍

Uh oh!

labkey-jeckels commented Jun 2, 2026

Uh oh!

labkey-adam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

labkey-jeckels commented Jun 2, 2026 •

edited

Loading