Skip to content

Don't defer any index creations during materialization #8023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-jeckels wants to merge 3 commits into
base: release26.7-SNAPSHOT
Choose a base branch
from
Open

Don't defer any index creations during materialization #8023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad529e2
Don't defer any index creations
labkey-jeckels Sep 7, 2026
673c091
Fix comment
labkey-jeckels Sep 7, 2026
cc239d3
Avoid caching misses when a subsequent row could cause it to become a…
labkey-jeckels Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions api/src/org/labkey/api/dataiterator/SimpleTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,18 +203,26 @@ 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<Pair<ColumnInfo, ColumnInfo>> _bulkLoads = new HashSet<>();

private List<Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?, ?>>> _maps = null;
private Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?, ?>> _titleColumnLookupMap = null;
private Pair<ColumnInfo, Map<?, ?>> _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)
Expand Down Expand Up @@ -371,11 +379,14 @@ private Object fetch(Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?,?>> 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);
Expand Down Expand Up @@ -414,6 +425,7 @@ private Object fetch(Pair<ColumnInfo, Map<?,?>> 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;
}
Expand All @@ -434,7 +446,8 @@ private Object fetch(Pair<ColumnInfo, Map<?,?>> pair, Object k)
return map.get(k);
else
{
map.put(k, MISS);
if (_cacheMisses)
map.put(k, MISS);
return null;
}
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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()
{
Expand Down
7 changes: 3 additions & 4 deletions experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)");

@labkey-nicka labkey-nicka Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for this line is now incorrect.


getDomainIndexDdl().forEach(builder::addDeferredIndex);
getDomainIndexDdl().forEach(builder::addIndex);

if (isIncrementalUpdateDisabled())
builder.addInvalidCheck(() -> String.valueOf(getInvalidateCounters(_ss.getLSID()).update.get()));
Expand Down