diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index b17fc536299..32a0943dc06 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -203,6 +203,7 @@ public static class RemapConverter private boolean _includePkLookup; // if true, will perform an initial PK lookup before attempting the AK lookup private final boolean _allowBulkLoads; + private final boolean _cacheMisses; private final Set> _bulkLoads = new HashSet<>(); private List>> _maps = null; @@ -210,11 +211,18 @@ public static class RemapConverter private Pair> _pkColumnLookupMap = null; public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn, boolean allowBulkLoads, boolean includePkLookup) + { + this(targetTable, includeTitleColumn, allowBulkLoads, includePkLookup, true); + } + + /** @param cacheMisses false when the same operation writes the lookup target, so an unresolved key must be re-queried instead of memoized */ + public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn, boolean allowBulkLoads, boolean includePkLookup, boolean cacheMisses) { _targetTable = targetTable; _includeTitleColumn = includeTitleColumn; _allowBulkLoads = allowBulkLoads; _includePkLookup = includePkLookup; + _cacheMisses = cacheMisses; } public void setIncludePkLookup(boolean includePkLookup) @@ -371,11 +379,14 @@ private Object fetch(Triple> triple, vs = bulkLoaded; } - // ArrayListValuedHashMap returns an empty collection if 'k' is not in the map. - // If there are no values in the database, stash a MISS marker to avoid re-fetching. assert vs != null; if (vs.isEmpty()) + { + if (!_cacheMisses) + return null; + map.put(k, MISS); + } } Object v = getSingleValue(k, vs); @@ -414,6 +425,7 @@ private Object fetch(Pair> pair, Object k) { if (k == null || (k instanceof String strKey && !GUID.isGUID(strKey))) { + // Not a data miss: a key that isn't a GUID can never become one, so memoize regardless of _cacheMisses map.put(k, MISS); return null; } @@ -434,7 +446,8 @@ private Object fetch(Pair> pair, Object k) return map.get(k); else { - map.put(k, MISS); + if (_cacheMisses) + map.put(k, MISS); return null; } } @@ -949,7 +962,8 @@ public RemappingConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _toCol = toCol; _missing = missing; _includeTitleColumn = includeTitleColumn; - _remapper = new RemapConverter(_toCol.getFkTableInfo(), _includeTitleColumn, false, true); + // The import can create the rows this resolves against, so a miss must not outlive the row that saw it + _remapper = new RemapConverter(_toCol.getFkTableInfo(), _includeTitleColumn, false, true, false); _lookupResolutionType = lookupResolutionType; } @@ -2330,6 +2344,33 @@ public void remapPkLookupMapIsRetained() assertSame("pk lookup map was rebuilt rather than retained", pkMap, converter.pkLookupMap()); } + @Test + public void remapMissIsNotMemoizedWhenCacheMissesIsOff() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true, false); + MultiValuedMap cache = converter.getMaps().getFirst().getRight(); + + String absent = "no-enum-value-supplies-this"; + assertNull(converter.mappedValue(absent)); + assertFalse("miss was memoized, so a row the import adds later can never resolve", cache.containsKey(absent)); + + // Stands in for the row appearing after the first lookup + Integer added = 42; + cache.put(absent, added); + assertEquals(added, converter.mappedValue(absent)); + } + + @Test + public void remapMissIsMemoizedByDefault() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + MultiValuedMap cache = converter.getMaps().getFirst().getRight(); + + String absent = "no-enum-value-supplies-this"; + assertNull(converter.mappedValue(absent)); + assertTrue("callers that don't write the lookup target rely on the MISS marker to avoid re-querying", cache.containsKey(absent)); + } + @Test public void getFileRootSubstitutedFilePathTest() { diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 6ff9ce9f4f0..e6cb7707a7c 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -1373,11 +1373,10 @@ private _MaterializedQueryHelper getOrCreateMQH() // be added as a followup step .addIndex("CREATE UNIQUE INDEX uq_${NAME}_rowid ON temp.${NAME} (rowid)") .addIndex("CREATE INDEX idx_${NAME}_container ON temp.${NAME} (container)") - .addDeferredIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)") - // Deferred despite being UNIQUE. Source data guarantees uniqueness, and this is very expensive to build - .addDeferredIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)"); + .addIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)") + .addIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)"); - getDomainIndexDdl().forEach(builder::addDeferredIndex); + getDomainIndexDdl().forEach(builder::addIndex); if (isIncrementalUpdateDisabled()) builder.addInvalidCheck(() -> String.valueOf(getInvalidateCounters(_ss.getLSID()).update.get()));