Skip to content

Revert optimization to SimpleTranslator #8028

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

Merged
labkey-jeckels merged 1 commit into from
Sep 8, 2026
Merged

Revert optimization to SimpleTranslator #8028

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
113 changes: 8 additions & 105 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 @@ -220,24 +220,14 @@ public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn
public void setIncludePkLookup(boolean includePkLookup)
{
_includePkLookup = includePkLookup;
_maps = null;
}

public ColumnInfo getPkColumn()
{
return _targetTable.getPkColumns().getFirst();
}

private Pair<ColumnInfo, Map<?, ?>> pkLookupMap()
{
if (!_includePkLookup)
return null;

if (_pkColumnLookupMap == null)
_pkColumnLookupMap = Pair.of(getPkColumn(), new HashMap<>());

return _pkColumnLookupMap;
}

private List<Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?, ?>>> getMaps()
{
if (_maps == null)
Expand Down Expand Up @@ -281,6 +271,11 @@ public ColumnInfo getPkColumn()
_titleColumnLookupMap = Triple.of(pkCol, titleColumn, new ArrayListValuedHashMap());
}
}

if (_includePkLookup)
{
_pkColumnLookupMap = Pair.of(pkCol, new HashMap<>());
}
}
return _maps;
}
Expand All @@ -296,10 +291,9 @@ public Object mappedValue(Object k)

List<Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?,?>>> maps = getMaps();

Pair<ColumnInfo, Map<?, ?>> pkLookupMap = pkLookupMap();
if (pkLookupMap != null)
if (_pkColumnLookupMap != null)
{
Object v = fetch(pkLookupMap, k);
Object v = fetch(_pkColumnLookupMap, k);
if (v != null)
return v;
}
Expand Down Expand Up @@ -2239,97 +2233,6 @@ public void convertRemapTest() throws Exception

}

/** Lookup fixture with one text alternate key (Value) over an integer pk (RowId); Ordinal is unique but not text, so it yields no map. */
private EnumTableInfo<LookupValues> remapLookupTable()
{
var core = QueryService.get().getUserSchema(TestContext.get().getUser(), JunitUtil.getTestContainer(), "core");
return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true);
}

@Test
public void remapCacheSurvivesPkLookupToggle()
{
RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true);

// RemappingConvertColumn flips this before every row, so it must not discard what earlier rows resolved
converter.setIncludePkLookup(false);

List<Triple<ColumnInfo, ColumnInfo, MultiValuedMap<?, ?>>> maps = converter.getMaps();
assertEquals("expected one alternate-key map, on the Value column", 1, maps.size());

// Seed keys no enum value can supply, so anything but a cache hit resolves to null
MultiValuedMap cache = maps.getFirst().getRight();
Integer seeded = 42;
cache.put("seeded-hit", seeded);
cache.put("seeded-miss", converter.MISS);

assertEquals(seeded, converter.mappedValue("seeded-hit"));
assertNull(converter.mappedValue("seeded-miss"));

for (int i = 0; i < 3; i++)
{
converter.setIncludePkLookup(true);
converter.setIncludePkLookup(false);
}

assertSame("toggling includePkLookup discarded the cached lookups", maps, converter.getMaps());
assertEquals("resolved value was discarded, so every row re-queries it", seeded, converter.mappedValue("seeded-hit"));
assertNull("MISS marker was discarded, so every row re-queries the absent value", converter.mappedValue("seeded-miss"));
}

@Test
public void remapResolutionIsStableAcrossPkLookupToggle()
{
RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true);
converter.setIncludePkLookup(false);

Object resolved = converter.mappedValue(LookupValues.Two.name());
assertNotNull("expected " + LookupValues.Two + " to resolve by alternate key", resolved);

converter.setIncludePkLookup(true);
converter.setIncludePkLookup(false);

assertEquals(resolved, converter.mappedValue(LookupValues.Two.name()));
}

@Test
public void remapAlternateKeyWinsWhenPkLookupIsOff()
{
RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true);

// Seed the two maps to disagree on one key, so the resolved value says which map was consulted
Integer key = 7;
Integer pkResolution = 7;
Integer akResolution = 99;
Map pkCache = converter.pkLookupMap().getValue();
MultiValuedMap akCache = converter.getMaps().getFirst().getRight();
pkCache.put(key, pkResolution);
akCache.put(key, akResolution);

assertEquals("pk lookup should take precedence while includePkLookup is on", pkResolution, converter.mappedValue(key));

// The pk map survives the toggle, so this also pins that it is not consulted while the flag is off
converter.setIncludePkLookup(false);
assertEquals("alternate key should resolve while includePkLookup is off", akResolution, converter.mappedValue(key));
}

@Test
public void remapPkLookupMapIsRetained()
{
RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, false);
assertNull("pk lookup map should not exist while includePkLookup is off", converter.pkLookupMap());

converter.setIncludePkLookup(true);
Pair<ColumnInfo, Map<?, ?>> pkMap = converter.pkLookupMap();
assertNotNull(pkMap);

converter.setIncludePkLookup(false);
assertNull(converter.pkLookupMap());

converter.setIncludePkLookup(true);
assertSame("pk lookup map was rebuilt rather than retained", pkMap, converter.pkLookupMap());
}

@Test
public void getFileRootSubstitutedFilePathTest()
{
Expand Down