Skip to content

Commit de0b2f0

Browse files
cpwrightcpwright
authored
fix: DH-18539: Fix incorrect snapshot results on historical sorted rollups. (#6648)
1 parent 3f0411e commit de0b2f0

File tree

6 files changed

+136
-31
lines changed

6 files changed

+136
-31
lines changed

‎engine/api/src/main/java/io/deephaven/engine/table/Table.java

+6
Original file line numberDiff line numberDiff line change
@@ -179,10 +179,16 @@ public interface Table extends
179179
* implementation.
180180
*/
181181
String AGGREGATION_ROW_LOOKUP_ATTRIBUTE = "AggregationRowLookup";
182+
182183
/**
183184
* Attribute on sort results used for hierarchical table construction. Specification is left to the implementation.
184185
*/
185186
String SORT_REVERSE_LOOKUP_ATTRIBUTE = "SortReverseLookup";
187+
/**
188+
* Attribute on sort results used for hierarchical table construction. Specification is left to the implementation.
189+
*/
190+
String SORT_ROW_REDIRECTION_ATTRIBUTE = "SortRowRedirection";
191+
186192
String SNAPSHOT_VIEWPORT_TYPE = "Snapshot";
187193
/**
188194
* This attribute is used internally by TableTools.merge to detect successive merges. Its presence indicates that it

‎engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java

+40-22
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,21 @@
2626
import org.jetbrains.annotations.NotNull;
2727
import org.jetbrains.annotations.Nullable;
2828

29-
import java.util.Arrays;
30-
import java.util.LinkedHashMap;
31-
import java.util.Map;
29+
import java.util.*;
3230
import java.util.function.LongUnaryOperator;
3331

3432
import static io.deephaven.engine.table.Table.SORT_REVERSE_LOOKUP_ATTRIBUTE;
33+
import static io.deephaven.engine.table.Table.SORT_ROW_REDIRECTION_ATTRIBUTE;
3534

3635
public class SortOperation implements QueryTable.MemoizableOperation<QueryTable> {
36+
static final Map<String, Object> IDENTITY_REDIRECTION_ATTRIBUTES;
37+
// The "+" sign is not valid in a column, therefore we can be sure that this is a proper sentinel value.
38+
static final String IDENTITY_REDIRECTION_VALUE = "+IDENTITY_REDIRECTION";
39+
static {
40+
final HashMap<String, Object> identityRedirectionAttributes = new HashMap<>();
41+
identityRedirectionAttributes.put(SORT_ROW_REDIRECTION_ATTRIBUTE, IDENTITY_REDIRECTION_VALUE);
42+
IDENTITY_REDIRECTION_ATTRIBUTES = Collections.unmodifiableMap(identityRedirectionAttributes);
43+
}
3744

3845
private final QueryTable parent;
3946
private QueryTable resultTable;
@@ -135,14 +142,12 @@ private QueryTable historicalSort(SortHelpers.SortMapping sortedKeys) {
135142
final TrackingRowSet resultRowSet = RowSetFactory.flat(sortedKeys.size()).toTracking();
136143

137144
final Map<String, ColumnSource<?>> resultMap = new LinkedHashMap<>();
138-
for (Map.Entry<String, ColumnSource<?>> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) {
139-
resultMap.put(stringColumnSourceEntry.getKey(),
140-
RedirectedColumnSource.maybeRedirect(sortMapping, stringColumnSourceEntry.getValue()));
141-
}
145+
final String sortMappingColumnName = populateRedirectedColumns(resultMap, sortMapping);
142146

143147
resultTable = new QueryTable(resultRowSet, resultMap);
144148
parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort);
145149
resultTable.setFlat();
150+
resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMappingColumnName);
146151
setSorted(resultTable);
147152
return resultTable;
148153
}
@@ -228,7 +233,8 @@ private void setSorted(QueryTable table) {
228233
}
229234

230235
private QueryTable withSorted(QueryTable table) {
231-
return (QueryTable) SortedColumnsAttribute.withOrderForColumn(table, sortColumnNames[0], sortOrder[0]);
236+
return (QueryTable) SortedColumnsAttribute.withOrderForColumn(table, sortColumnNames[0], sortOrder[0],
237+
IDENTITY_REDIRECTION_ATTRIBUTES);
232238
}
233239

234240
@Override
@@ -280,10 +286,7 @@ public Result<QueryTable> initialize(boolean usePrev, long beforeClock) {
280286
sortMapping.writableCast().fillFromChunk(fillFromContext, LongChunk.chunkWrap(sortedKeys),
281287
closer.add(resultRowSet.copy()));
282288

283-
for (Map.Entry<String, ColumnSource<?>> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) {
284-
resultMap.put(stringColumnSourceEntry.getKey(),
285-
RedirectedColumnSource.maybeRedirect(sortMapping, stringColumnSourceEntry.getValue()));
286-
}
289+
String sortMappingColumnName = populateRedirectedColumns(resultMap, sortMapping);
287290

288291
// noinspection unchecked
289292
final ColumnSource<Comparable<?>>[] sortedColumnsToSortBy =
@@ -298,6 +301,7 @@ public Result<QueryTable> initialize(boolean usePrev, long beforeClock) {
298301

299302
resultTable = new QueryTable(resultRowSet, resultMap);
300303
parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort);
304+
resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMappingColumnName);
301305
setReverseLookup(resultTable, (final long innerRowKey) -> {
302306
final long outerRowKey = reverseLookup.get(innerRowKey);
303307
return outerRowKey == reverseLookup.getNoEntryValue() ? RowSequence.NULL_ROW_KEY : outerRowKey;
@@ -320,19 +324,34 @@ public Result<QueryTable> initialize(boolean usePrev, long beforeClock) {
320324
}
321325
}
322326

327+
private String populateRedirectedColumns(Map<String, ColumnSource<?>> resultMap, RowRedirection sortMapping) {
328+
// if nothing is actually redirected, we can use the identity value
329+
String sortMappingColumnName = IDENTITY_REDIRECTION_VALUE;
330+
331+
for (Map.Entry<String, ColumnSource<?>> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) {
332+
final ColumnSource<?> innerSource = stringColumnSourceEntry.getValue();
333+
final ColumnSource<?> redirectedSource = RedirectedColumnSource.maybeRedirect(sortMapping, innerSource);
334+
resultMap.put(stringColumnSourceEntry.getKey(), redirectedSource);
335+
if (redirectedSource != innerSource) {
336+
sortMappingColumnName = stringColumnSourceEntry.getKey();
337+
}
338+
}
339+
return sortMappingColumnName;
340+
}
341+
323342
/**
324343
* Get the row redirection for a sort result.
325344
*
326345
* @param sortResult The sort result table; <em>must</em> be the direct result of a sort.
327-
* @return The row redirection if at least one column required redirection, otherwise {@code null}
346+
* @return The row redirection for this table if at least one column required redirection, otherwise {@code null}
328347
*/
329348
public static RowRedirection getRowRedirection(@NotNull final Table sortResult) {
330-
for (final ColumnSource<?> columnSource : sortResult.getColumnSources()) {
331-
if (columnSource instanceof RedirectedColumnSource) {
332-
return ((RedirectedColumnSource<?>) columnSource).getRowRedirection();
333-
}
349+
final String columnName = (String) sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE);
350+
if (columnName == null || columnName.equals(IDENTITY_REDIRECTION_VALUE)) {
351+
return null;
334352
}
335-
return null;
353+
354+
return ((RedirectedColumnSource<?>) sortResult.getColumnSource(columnName)).getRowRedirection();
336355
}
337356

338357
/**
@@ -351,7 +370,7 @@ public static RowRedirection getRowRedirection(@NotNull final Table sortResult)
351370
*
352371
* @param parent The sort input table; must have been sorted in order to produce {@code sortResult}
353372
* @param sortResult The sort result table; <em>must</em> be the direct result of a sort on {@code parent}
354-
* @return The reverse lookup
373+
* @return The reverse lookup, or null if no redirection is performed.
355374
*/
356375
public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @NotNull final Table sortResult) {
357376
if (BlinkTableTools.isBlink(parent)) {
@@ -365,9 +384,8 @@ public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @N
365384
return (LongUnaryOperator) value;
366385
}
367386
final RowRedirection sortRedirection = getRowRedirection(sortResult);
368-
if (sortRedirection == null || sortRedirection == getRowRedirection(parent)) {
369-
// Static table was already sorted
370-
return LongUnaryOperator.identity();
387+
if (sortRedirection == null) {
388+
return null;
371389
}
372390
final HashMapK4V4 reverseLookup = new HashMapLockFreeK4V4(sortResult.intSize(), .75f, RowSequence.NULL_ROW_KEY);
373391
try (final LongColumnIterator innerRowKeys =

‎engine/table/src/main/java/io/deephaven/engine/table/impl/SortedColumnsAttribute.java

+22
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,28 @@ public static Table withOrderForColumn(Table table, String columnName, SortingOr
9797
return table.withAttributes(Map.of(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute));
9898
}
9999

100+
/**
101+
* Ensure that the result table is marked as sorted by the given column.
102+
*
103+
* @param table the table to update
104+
* @param columnName the column to update
105+
* @param order the order that the column is sorted in
106+
* @return {@code table}, or a copy of it with the necessary attribute set
107+
*/
108+
public static Table withOrderForColumn(Table table, String columnName, SortingOrder order,
109+
Map<String, ?> additionalAttributes) {
110+
final String oldAttribute = (String) table.getAttribute(Table.SORTED_COLUMNS_ATTRIBUTE);
111+
final String newAttribute = setOrderForColumn(oldAttribute, columnName, order);
112+
if (additionalAttributes.isEmpty()) {
113+
return table.withAttributes(Map.of(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute));
114+
} else {
115+
final Map<String, Object> attributesToAdd = new LinkedHashMap<>();
116+
attributesToAdd.putAll(additionalAttributes);
117+
attributesToAdd.put(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute);
118+
return table.withAttributes(attributesToAdd);
119+
}
120+
}
121+
100122
/**
101123
* Get the columns a {@link Table} is sorted by.
102124
*

‎engine/table/src/main/java/io/deephaven/engine/table/impl/sources/RedirectedColumnSource.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,7 @@ private void doFillChunk(@NotNull final ColumnSource.FillContext context,
523523

524524
if (ascendingMapping) {
525525
effectiveContext.doOrderedFillAscending(innerSource, usePrev, destination);
526-
} else if (innerSource instanceof FillUnordered) {
526+
} else if (FillUnordered.providesFillUnordered(innerSource)) {
527527
// noinspection unchecked
528528
effectiveContext.doUnorderedFill((FillUnordered<Values>) innerSource, usePrev, destination);
529529
} else {

‎engine/table/src/test/java/io/deephaven/engine/table/impl/TestHierarchicalTableSnapshots.java

+66-7
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,15 @@
44
package io.deephaven.engine.table.impl;
55

66
import io.deephaven.api.ColumnName;
7+
import io.deephaven.api.agg.Aggregation;
8+
import io.deephaven.api.agg.spec.AggSpec;
79
import io.deephaven.chunk.WritableChunk;
810
import io.deephaven.chunk.attributes.Values;
11+
import io.deephaven.csv.CsvTools;
12+
import io.deephaven.csv.util.CsvReaderException;
913
import io.deephaven.engine.context.ExecutionContext;
1014
import io.deephaven.engine.rowset.RowSequence;
15+
import io.deephaven.engine.rowset.RowSequenceFactory;
1116
import io.deephaven.engine.rowset.RowSetFactory;
1217
import io.deephaven.engine.rowset.RowSetShiftData;
1318
import io.deephaven.engine.table.*;
@@ -20,14 +25,17 @@
2025
import io.deephaven.engine.table.impl.sources.chunkcolumnsource.ChunkColumnSource;
2126
import io.deephaven.engine.testutil.ControlledUpdateGraph;
2227
import io.deephaven.engine.testutil.junit4.EngineCleanup;
28+
import io.deephaven.engine.util.TableTools;
2329
import io.deephaven.test.types.OutOfBandTest;
2430

31+
import junit.framework.TestCase;
2532
import org.jetbrains.annotations.NotNull;
2633
import org.jetbrains.annotations.Nullable;
2734
import org.junit.Rule;
2835
import org.junit.Test;
2936
import org.junit.experimental.categories.Category;
3037

38+
import java.io.ByteArrayInputStream;
3139
import java.time.Instant;
3240
import java.util.Arrays;
3341
import java.util.BitSet;
@@ -44,13 +52,8 @@
4452
import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.byteToBooleanSource;
4553
import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.longToInstantSource;
4654
import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.maybeConvertToPrimitiveChunkType;
47-
import static io.deephaven.engine.testutil.TstUtils.addToTable;
48-
import static io.deephaven.engine.testutil.TstUtils.assertTableEquals;
49-
import static io.deephaven.engine.testutil.TstUtils.testRefreshingTable;
50-
import static io.deephaven.engine.util.TableTools.booleanCol;
51-
import static io.deephaven.engine.util.TableTools.byteCol;
52-
import static io.deephaven.engine.util.TableTools.intCol;
53-
import static io.deephaven.engine.util.TableTools.newTable;
55+
import static io.deephaven.engine.testutil.TstUtils.*;
56+
import static io.deephaven.engine.util.TableTools.*;
5457
import static io.deephaven.util.QueryConstants.NULL_INT;
5558
import static org.assertj.core.api.Assertions.assertThat;
5659

@@ -201,6 +204,62 @@ public void testTreeSnapshotSatisfaction() throws ExecutionException, Interrupte
201204
concurrentExecutor.shutdown();
202205
}
203206

207+
@Test
208+
public void testSortedExpandAll() throws CsvReaderException {
209+
final String data = "A,B,C,N\n" +
210+
"Apple,One,Alpha,1\n" +
211+
"Apple,One,Alpha,2\n" +
212+
"Apple,One,Bravo,3\n" +
213+
"Apple,One,Bravo,4\n" +
214+
"Apple,One,Bravo,5\n" +
215+
"Apple,One,Bravo,6\n" +
216+
"Banana,Two,Alpha,7\n" +
217+
"Banana,Two,Alpha,8\n" +
218+
"Banana,Two,Bravo,3\n" +
219+
"Banana,Two,Bravo,4\n" +
220+
"Banana,Three,Bravo,1\n" +
221+
"Banana,Three,Bravo,1\n";
222+
223+
final Table source = CsvTools.readCsv(new ByteArrayInputStream(data.getBytes()));
224+
225+
TableTools.show(source);
226+
final RollupTable rollupTable = source.rollup(List.of(Aggregation.of(AggSpec.sum(), "N")), "A", "B", "C");
227+
final RollupTable sortedRollup = rollupTable.withNodeOperations(
228+
rollupTable.makeNodeOperationsRecorder(RollupTable.NodeType.Aggregated).sortDescending("N"));
229+
230+
final String[] arrayWithNull = new String[1];
231+
final Table keyTable = newTable(
232+
intCol(rollupTable.getRowDepthColumn().name(), 0),
233+
stringCol("A", arrayWithNull),
234+
stringCol("B", arrayWithNull),
235+
stringCol("C", arrayWithNull),
236+
byteCol("Action", HierarchicalTable.KEY_TABLE_ACTION_EXPAND_ALL));
237+
238+
final SnapshotState ss = rollupTable.makeSnapshotState();
239+
final Table snapshot =
240+
snapshotToTable(rollupTable, ss, keyTable, ColumnName.of("Action"), null, RowSetFactory.flat(30));
241+
TableTools.showWithRowSet(snapshot);
242+
243+
final SnapshotState ssSort = sortedRollup.makeSnapshotState();
244+
245+
final Table snapshotSort =
246+
snapshotToTable(sortedRollup, ssSort, keyTable, ColumnName.of("Action"), null, RowSetFactory.flat(30));
247+
TableTools.showWithRowSet(snapshotSort);
248+
249+
// first we know that the size of the tables must be the same
250+
TestCase.assertEquals(snapshot.size(), snapshotSort.size());
251+
// and the first row must be the same, because it is the parent
252+
assertTableEquals(snapshot.head(1), snapshotSort.head(1));
253+
// then we have six rows of banana, and that should be identical
254+
assertTableEquals(snapshot.slice(5, 11), snapshotSort.slice(1, 7));
255+
// then we need to check on the apple rows, but those are not actually identical because of sorting
256+
Table appleExpected = snapshot.where("A=`Apple`").sortDescending("N");
257+
assertTableEquals(appleExpected, snapshotSort.slice(7, 11));
258+
259+
freeSnapshotTableChunks(snapshot);
260+
freeSnapshotTableChunks(snapshotSort);
261+
}
262+
204263
@SuppressWarnings("SameParameterValue")
205264
private static Table snapshotToTable(
206265
@NotNull final HierarchicalTable<?> hierarchicalTable,

‎go/pkg/client/example_import_table_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func Example_importTable() {
9393
// metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "float"]
9494
// - Volume: type=int32, nullable
9595
// metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "int"]
96-
// metadata: ["deephaven:attribute.AddOnly": "true", "deephaven:attribute.AppendOnly": "true", "deephaven:attribute.SortedColumns": "Close=Ascending", "deephaven:attribute_type.AddOnly": "java.lang.Boolean", "deephaven:attribute_type.AppendOnly": "java.lang.Boolean", "deephaven:attribute_type.SortedColumns": "java.lang.String"]
96+
// metadata: ["deephaven:attribute.AddOnly": "true", "deephaven:attribute.AppendOnly": "true", "deephaven:attribute.SortRowRedirection": "Volume", "deephaven:attribute.SortedColumns": "Close=Ascending", "deephaven:attribute_type.AddOnly": "java.lang.Boolean", "deephaven:attribute_type.AppendOnly": "java.lang.Boolean", "deephaven:attribute_type.SortRowRedirection": "java.lang.String", "deephaven:attribute_type.SortedColumns": "java.lang.String"]
9797
// rows: 5
9898
// col[0][Ticker]: ["IBM" "XRX" "XYZZY" "GME" "ZNGA"]
9999
// col[1][Close]: [38.7 53.8 88.5 453 544.9]

0 commit comments

Comments
 (0)