From 1f397ffc8738137b8fd5ecd743cdd6abbe87eabd Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 10:10:57 -0500 Subject: [PATCH 01/14] fix: DH-18539: Fix incorrect snapshot results on historical sorted rollups --- .../java/io/deephaven/engine/table/Table.java | 6 ++ .../engine/table/impl/SortOperation.java | 33 +++++---- .../table/impl/SortedColumnsAttribute.java | 22 ++++++ .../impl/sources/RedirectedColumnSource.java | 2 +- .../impl/util/IdentityRowRedirection.java | 69 ++++++++++++++++++ .../impl/TestHierarchicalTableSnapshots.java | 73 +++++++++++++++++-- 6 files changed, 183 insertions(+), 22 deletions(-) create mode 100644 engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java diff --git a/engine/api/src/main/java/io/deephaven/engine/table/Table.java b/engine/api/src/main/java/io/deephaven/engine/table/Table.java index f3a23223a07..435ab1d89ba 100644 --- a/engine/api/src/main/java/io/deephaven/engine/table/Table.java +++ b/engine/api/src/main/java/io/deephaven/engine/table/Table.java @@ -179,10 +179,16 @@ public interface Table extends * implementation. */ String AGGREGATION_ROW_LOOKUP_ATTRIBUTE = "AggregationRowLookup"; + /** * Attribute on sort results used for hierarchical table construction. Specification is left to the implementation. */ String SORT_REVERSE_LOOKUP_ATTRIBUTE = "SortReverseLookup"; + /** + * Attribute on sort results used for hierarchical table construction. Specificaiton is left to the implementation. + */ + String SORT_ROW_REDIRECTION_ATTRIBUTE = "SortRowRedirection"; + String SNAPSHOT_VIEWPORT_TYPE = "Snapshot"; /** * This attribute is used internally by TableTools.merge to detect successive merges. Its presence indicates that it diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index 616100a487a..1fbbf64135a 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -14,6 +14,7 @@ import io.deephaven.engine.table.impl.sources.ReinterpretUtils; import io.deephaven.engine.table.impl.sources.SwitchColumnSource; import io.deephaven.engine.table.impl.sources.chunkcolumnsource.LongChunkColumnSource; +import io.deephaven.engine.table.impl.util.IdentityRowRedirection; import io.deephaven.engine.table.impl.util.LongColumnSourceRowRedirection; import io.deephaven.engine.table.impl.util.RowRedirection; import io.deephaven.engine.table.impl.util.WritableRowRedirection; @@ -26,14 +27,20 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.Arrays; -import java.util.LinkedHashMap; -import java.util.Map; +import java.util.*; import java.util.function.LongUnaryOperator; import static io.deephaven.engine.table.Table.SORT_REVERSE_LOOKUP_ATTRIBUTE; +import static io.deephaven.engine.table.Table.SORT_ROW_REDIRECTION_ATTRIBUTE; public class SortOperation implements QueryTable.MemoizableOperation { + static final Map IDENTITY_REDIRECTION_ATTRIBUTES; + static { + final HashMap identityRedirectionAttributes = new HashMap<>(); + identityRedirectionAttributes.put(SORT_REVERSE_LOOKUP_ATTRIBUTE, LongUnaryOperator.identity()); + identityRedirectionAttributes.put(SORT_ROW_REDIRECTION_ATTRIBUTE, IdentityRowRedirection.INSTANCE); + IDENTITY_REDIRECTION_ATTRIBUTES = Collections.unmodifiableMap(identityRedirectionAttributes); + } private final QueryTable parent; private QueryTable resultTable; @@ -143,6 +150,7 @@ private QueryTable historicalSort(SortHelpers.SortMapping sortedKeys) { resultTable = new QueryTable(resultRowSet, resultMap); parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort); resultTable.setFlat(); + resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMapping); setSorted(resultTable); return resultTable; } @@ -228,7 +236,8 @@ private void setSorted(QueryTable table) { } private QueryTable withSorted(QueryTable table) { - return (QueryTable) SortedColumnsAttribute.withOrderForColumn(table, sortColumnNames[0], sortOrder[0]); + return (QueryTable) SortedColumnsAttribute.withOrderForColumn(table, sortColumnNames[0], sortOrder[0], + IDENTITY_REDIRECTION_ATTRIBUTES); } @Override @@ -298,6 +307,7 @@ public Result initialize(boolean usePrev, long beforeClock) { resultTable = new QueryTable(resultRowSet, resultMap); parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort); + resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMapping); setReverseLookup(resultTable, (final long innerRowKey) -> { final long outerRowKey = reverseLookup.get(innerRowKey); return outerRowKey == reverseLookup.getNoEntryValue() ? RowSequence.NULL_ROW_KEY : outerRowKey; @@ -324,15 +334,14 @@ public Result initialize(boolean usePrev, long beforeClock) { * Get the row redirection for a sort result. * * @param sortResult The sort result table; must be the direct result of a sort. - * @return The row redirection if at least one column required redirection, otherwise {@code null} + * @return The row redirection for this table. */ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) { - for (final ColumnSource columnSource : sortResult.getColumnSources()) { - if (columnSource instanceof RedirectedColumnSource) { - return ((RedirectedColumnSource) columnSource).getRowRedirection(); - } + final RowRedirection attribute = (RowRedirection) sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); + if (attribute == null) { + throw new IllegalStateException("getRowRedirection argument is not the result of a sort."); } - return null; + return attribute; } /** @@ -365,10 +374,6 @@ public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @N return (LongUnaryOperator) value; } final RowRedirection sortRedirection = getRowRedirection(sortResult); - if (sortRedirection == null || sortRedirection == getRowRedirection(parent)) { - // Static table was already sorted - return LongUnaryOperator.identity(); - } final HashMapK4V4 reverseLookup = new HashMapLockFreeK4V4(sortResult.intSize(), .75f, RowSequence.NULL_ROW_KEY); try (final LongColumnIterator innerRowKeys = new ChunkedLongColumnIterator(sortRedirection, sortResult.getRowSet()); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortedColumnsAttribute.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortedColumnsAttribute.java index 4f1a895ae89..63444fb4e4a 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortedColumnsAttribute.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortedColumnsAttribute.java @@ -97,6 +97,28 @@ public static Table withOrderForColumn(Table table, String columnName, SortingOr return table.withAttributes(Map.of(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute)); } + /** + * Ensure that the result table is marked as sorted by the given column. + * + * @param table the table to update + * @param columnName the column to update + * @param order the order that the column is sorted in + * @return {@code table}, or a copy of it with the necessary attribute set + */ + public static Table withOrderForColumn(Table table, String columnName, SortingOrder order, + Map additionalAttributes) { + final String oldAttribute = (String) table.getAttribute(Table.SORTED_COLUMNS_ATTRIBUTE); + final String newAttribute = setOrderForColumn(oldAttribute, columnName, order); + if (additionalAttributes.isEmpty()) { + return table.withAttributes(Map.of(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute)); + } else { + final Map attributesToAdd = new LinkedHashMap<>(); + attributesToAdd.putAll(additionalAttributes); + attributesToAdd.put(Table.SORTED_COLUMNS_ATTRIBUTE, newAttribute); + return table.withAttributes(attributesToAdd); + } + } + /** * Get the columns a {@link Table} is sorted by. * diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/sources/RedirectedColumnSource.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/sources/RedirectedColumnSource.java index 383367ff605..413fdc74b49 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/sources/RedirectedColumnSource.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/sources/RedirectedColumnSource.java @@ -523,7 +523,7 @@ private void doFillChunk(@NotNull final ColumnSource.FillContext context, if (ascendingMapping) { effectiveContext.doOrderedFillAscending(innerSource, usePrev, destination); - } else if (innerSource instanceof FillUnordered) { + } else if (FillUnordered.providesFillUnordered(innerSource)) { // noinspection unchecked effectiveContext.doUnorderedFill((FillUnordered) innerSource, usePrev, destination); } else { diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java new file mode 100644 index 00000000000..5b646238300 --- /dev/null +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java @@ -0,0 +1,69 @@ +// +// Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending +// +package io.deephaven.engine.table.impl.util; + +import io.deephaven.chunk.WritableChunk; +import io.deephaven.chunk.WritableLongChunk; +import io.deephaven.engine.rowset.RowSequence; +import io.deephaven.engine.rowset.chunkattributes.RowKeys; +import io.deephaven.engine.table.ChunkSource; +import io.deephaven.engine.table.SharedContext; +import org.jetbrains.annotations.NotNull; + +/** + * A row redirection which implements the identity transformation. + */ +public class IdentityRowRedirection implements RowRedirection { + public static IdentityRowRedirection INSTANCE = new IdentityRowRedirection(); + + // static use only + private IdentityRowRedirection() {} + + @Override + public long get(long outerRowKey) { + return outerRowKey; + } + + @Override + public long getPrev(long outerRowKey) { + return outerRowKey; + } + + @Override + public ChunkSource.FillContext makeFillContext(final int chunkCapacity, final SharedContext sharedContext) { + return new FillContext() { + @Override + public boolean supportsUnboundedFill() { + return true; + } + }; + } + + @Override + public void fillChunk(@NotNull final ChunkSource.FillContext fillContext, + @NotNull final WritableChunk innerRowKeys, + @NotNull final RowSequence outerRowKeys) { + final WritableLongChunk innerRowKeysTyped = innerRowKeys.asWritableLongChunk(); + innerRowKeysTyped.setSize(outerRowKeys.intSize()); + outerRowKeys.fillRowKeyChunk(innerRowKeysTyped); + } + + @Override + public void fillPrevChunk(@NotNull final ChunkSource.FillContext fillContext, + @NotNull final WritableChunk innerRowKeys, + @NotNull final RowSequence outerRowKeys) { + fillChunk(fillContext, innerRowKeys, outerRowKeys); + } + + @Override + public boolean ascendingMapping() { + return true; + } + + + @Override + public String toString() { + return getClass().getSimpleName(); + } +} diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/TestHierarchicalTableSnapshots.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/TestHierarchicalTableSnapshots.java index 4aad36e6185..389ca9ba1f5 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/TestHierarchicalTableSnapshots.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/TestHierarchicalTableSnapshots.java @@ -4,10 +4,15 @@ package io.deephaven.engine.table.impl; import io.deephaven.api.ColumnName; +import io.deephaven.api.agg.Aggregation; +import io.deephaven.api.agg.spec.AggSpec; import io.deephaven.chunk.WritableChunk; import io.deephaven.chunk.attributes.Values; +import io.deephaven.csv.CsvTools; +import io.deephaven.csv.util.CsvReaderException; import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.rowset.RowSequence; +import io.deephaven.engine.rowset.RowSequenceFactory; import io.deephaven.engine.rowset.RowSetFactory; import io.deephaven.engine.rowset.RowSetShiftData; import io.deephaven.engine.table.*; @@ -20,14 +25,17 @@ import io.deephaven.engine.table.impl.sources.chunkcolumnsource.ChunkColumnSource; import io.deephaven.engine.testutil.ControlledUpdateGraph; import io.deephaven.engine.testutil.junit4.EngineCleanup; +import io.deephaven.engine.util.TableTools; import io.deephaven.test.types.OutOfBandTest; +import junit.framework.TestCase; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import java.io.ByteArrayInputStream; import java.time.Instant; import java.util.Arrays; import java.util.BitSet; @@ -44,13 +52,8 @@ import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.byteToBooleanSource; import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.longToInstantSource; import static io.deephaven.engine.table.impl.sources.ReinterpretUtils.maybeConvertToPrimitiveChunkType; -import static io.deephaven.engine.testutil.TstUtils.addToTable; -import static io.deephaven.engine.testutil.TstUtils.assertTableEquals; -import static io.deephaven.engine.testutil.TstUtils.testRefreshingTable; -import static io.deephaven.engine.util.TableTools.booleanCol; -import static io.deephaven.engine.util.TableTools.byteCol; -import static io.deephaven.engine.util.TableTools.intCol; -import static io.deephaven.engine.util.TableTools.newTable; +import static io.deephaven.engine.testutil.TstUtils.*; +import static io.deephaven.engine.util.TableTools.*; import static io.deephaven.util.QueryConstants.NULL_INT; import static org.assertj.core.api.Assertions.assertThat; @@ -201,6 +204,62 @@ public void testTreeSnapshotSatisfaction() throws ExecutionException, Interrupte concurrentExecutor.shutdown(); } + @Test + public void testSortedExpandAll() throws CsvReaderException { + final String data = "A,B,C,N\n" + + "Apple,One,Alpha,1\n" + + "Apple,One,Alpha,2\n" + + "Apple,One,Bravo,3\n" + + "Apple,One,Bravo,4\n" + + "Apple,One,Bravo,5\n" + + "Apple,One,Bravo,6\n" + + "Banana,Two,Alpha,7\n" + + "Banana,Two,Alpha,8\n" + + "Banana,Two,Bravo,3\n" + + "Banana,Two,Bravo,4\n" + + "Banana,Three,Bravo,1\n" + + "Banana,Three,Bravo,1\n"; + + final Table source = CsvTools.readCsv(new ByteArrayInputStream(data.getBytes())); + + TableTools.show(source); + final RollupTable rollupTable = source.rollup(List.of(Aggregation.of(AggSpec.sum(), "N")), "A", "B", "C"); + final RollupTable sortedRollup = rollupTable.withNodeOperations( + rollupTable.makeNodeOperationsRecorder(RollupTable.NodeType.Aggregated).sortDescending("N")); + + final String[] arrayWithNull = new String[1]; + final Table keyTable = newTable( + intCol(rollupTable.getRowDepthColumn().name(), 0), + stringCol("A", arrayWithNull), + stringCol("B", arrayWithNull), + stringCol("C", arrayWithNull), + byteCol("Action", HierarchicalTable.KEY_TABLE_ACTION_EXPAND_ALL)); + + final SnapshotState ss = rollupTable.makeSnapshotState(); + final Table snapshot = + snapshotToTable(rollupTable, ss, keyTable, ColumnName.of("Action"), null, RowSetFactory.flat(30)); + TableTools.showWithRowSet(snapshot); + + final SnapshotState ssSort = sortedRollup.makeSnapshotState(); + + final Table snapshotSort = + snapshotToTable(sortedRollup, ssSort, keyTable, ColumnName.of("Action"), null, RowSetFactory.flat(30)); + TableTools.showWithRowSet(snapshotSort); + + // first we know that the size of the tables must be the same + TestCase.assertEquals(snapshot.size(), snapshotSort.size()); + // and the first row must be the same, because it is the parent + assertTableEquals(snapshot.head(1), snapshotSort.head(1)); + // then we have six rows of banana, and that should be identical + assertTableEquals(snapshot.slice(5, 11), snapshotSort.slice(1, 7)); + // then we need to check on the apple rows, but those are not actually identical because of sorting + Table appleExpected = snapshot.where("A=`Apple`").sortDescending("N"); + assertTableEquals(appleExpected, snapshotSort.slice(7, 11)); + + freeSnapshotTableChunks(snapshot); + freeSnapshotTableChunks(snapshotSort); + } + @SuppressWarnings("SameParameterValue") private static Table snapshotToTable( @NotNull final HierarchicalTable hierarchicalTable, From 6ce2885c75f2a3d14ab3cf5967d13aa133b17b20 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 12:02:02 -0500 Subject: [PATCH 02/14] Update go test. --- go/pkg/client/example_import_table_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/pkg/client/example_import_table_test.go b/go/pkg/client/example_import_table_test.go index 133f33445bc..13a549e3be8 100644 --- a/go/pkg/client/example_import_table_test.go +++ b/go/pkg/client/example_import_table_test.go @@ -93,7 +93,7 @@ func Example_importTable() { // metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "float"] // - Volume: type=int32, nullable // metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "int"] - // 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"] + // 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", "deephaven:unsent.attribute.SortRowRedirection": ""] // rows: 5 // col[0][Ticker]: ["IBM" "XRX" "XYZZY" "GME" "ZNGA"] // col[1][Close]: [38.7 53.8 88.5 453 544.9] From a402930dd03a7088c97f7e9584a49648e15a20f7 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 15:26:40 -0500 Subject: [PATCH 03/14] adjust timeouts. --- .../web/client/api/subscription/ViewportTestGwt.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java index ff94060016c..bf216ff14a2 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java @@ -97,7 +97,7 @@ public void testChangePendingViewport() { connect(tables) .then(table("staticTable")) .then(table -> { - delayTestFinish(5001); + delayTestFinish(11001); table.setViewport(0, 25, null); assertEquals(100.0, table.getSize()); @@ -174,7 +174,7 @@ public void testViewportOnUpdatingTable() { connect(tables) .then(table("growingBackward")) .then(table -> { - delayTestFinish(4000); + delayTestFinish(10000); // set up a viewport, and watch it show up, and tick once table.setViewport(0, 9, null); return assertUpdateReceived(table, viewportData -> { From a0ac36afd853ccf5b44e832d75de967a921e8481 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 15:59:24 -0500 Subject: [PATCH 04/14] Revert "adjust timeouts." This reverts commit a402930dd03a7088c97f7e9584a49648e15a20f7. --- .../web/client/api/subscription/ViewportTestGwt.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java index bf216ff14a2..ff94060016c 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java @@ -97,7 +97,7 @@ public void testChangePendingViewport() { connect(tables) .then(table("staticTable")) .then(table -> { - delayTestFinish(11001); + delayTestFinish(5001); table.setViewport(0, 25, null); assertEquals(100.0, table.getSize()); @@ -174,7 +174,7 @@ public void testViewportOnUpdatingTable() { connect(tables) .then(table("growingBackward")) .then(table -> { - delayTestFinish(10000); + delayTestFinish(4000); // set up a viewport, and watch it show up, and tick once table.setViewport(0, 9, null); return assertUpdateReceived(table, viewportData -> { From f32fd5ace2a746e31fd7c77421bbdef93dc9b5ca Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 16:21:30 -0500 Subject: [PATCH 05/14] use simpler attribute. --- .../engine/table/impl/SortOperation.java | 30 +++++--- .../impl/util/IdentityRowRedirection.java | 69 ------------------- 2 files changed, 20 insertions(+), 79 deletions(-) delete mode 100644 engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index 1fbbf64135a..97923bad984 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -14,7 +14,6 @@ import io.deephaven.engine.table.impl.sources.ReinterpretUtils; import io.deephaven.engine.table.impl.sources.SwitchColumnSource; import io.deephaven.engine.table.impl.sources.chunkcolumnsource.LongChunkColumnSource; -import io.deephaven.engine.table.impl.util.IdentityRowRedirection; import io.deephaven.engine.table.impl.util.LongColumnSourceRowRedirection; import io.deephaven.engine.table.impl.util.RowRedirection; import io.deephaven.engine.table.impl.util.WritableRowRedirection; @@ -35,10 +34,10 @@ public class SortOperation implements QueryTable.MemoizableOperation { static final Map IDENTITY_REDIRECTION_ATTRIBUTES; + static final Object IDENTITY_REDIRECTION_VALUE = new String(); static { final HashMap identityRedirectionAttributes = new HashMap<>(); - identityRedirectionAttributes.put(SORT_REVERSE_LOOKUP_ATTRIBUTE, LongUnaryOperator.identity()); - identityRedirectionAttributes.put(SORT_ROW_REDIRECTION_ATTRIBUTE, IdentityRowRedirection.INSTANCE); + identityRedirectionAttributes.put(SORT_ROW_REDIRECTION_ATTRIBUTE, IDENTITY_REDIRECTION_VALUE); IDENTITY_REDIRECTION_ATTRIBUTES = Collections.unmodifiableMap(identityRedirectionAttributes); } @@ -138,19 +137,26 @@ private QueryTable historicalSort(SortHelpers.SortMapping sortedKeys) { return withSorted(parent); } + // if nothing is actually redirected, we can use the identity value + Object sortMappingColumnName = IDENTITY_REDIRECTION_VALUE; + final WritableRowRedirection sortMapping = sortedKeys.makeHistoricalRowRedirection(); final TrackingRowSet resultRowSet = RowSetFactory.flat(sortedKeys.size()).toTracking(); final Map> resultMap = new LinkedHashMap<>(); for (Map.Entry> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) { - resultMap.put(stringColumnSourceEntry.getKey(), - RedirectedColumnSource.maybeRedirect(sortMapping, stringColumnSourceEntry.getValue())); + final ColumnSource innerSource = stringColumnSourceEntry.getValue(); + ColumnSource redirectedSource = RedirectedColumnSource.maybeRedirect(sortMapping, innerSource); + resultMap.put(stringColumnSourceEntry.getKey(), redirectedSource); + if (redirectedSource != innerSource) { + sortMappingColumnName = stringColumnSourceEntry.getKey(); + } } resultTable = new QueryTable(resultRowSet, resultMap); parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort); resultTable.setFlat(); - resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMapping); + resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMappingColumnName); setSorted(resultTable); return resultTable; } @@ -337,11 +343,12 @@ public Result initialize(boolean usePrev, long beforeClock) { * @return The row redirection for this table. */ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) { - final RowRedirection attribute = (RowRedirection) sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); - if (attribute == null) { - throw new IllegalStateException("getRowRedirection argument is not the result of a sort."); + final Object columnName = sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); + if (columnName == null || columnName == IDENTITY_REDIRECTION_VALUE) { + return null; } - return attribute; + + return ((RedirectedColumnSource)sortResult.getColumnSource((String)columnName)).getRowRedirection(); } /** @@ -374,6 +381,9 @@ public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @N return (LongUnaryOperator) value; } final RowRedirection sortRedirection = getRowRedirection(sortResult); + if (sortRedirection == null) { + return null; + } final HashMapK4V4 reverseLookup = new HashMapLockFreeK4V4(sortResult.intSize(), .75f, RowSequence.NULL_ROW_KEY); try (final LongColumnIterator innerRowKeys = new ChunkedLongColumnIterator(sortRedirection, sortResult.getRowSet()); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java deleted file mode 100644 index 5b646238300..00000000000 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/util/IdentityRowRedirection.java +++ /dev/null @@ -1,69 +0,0 @@ -// -// Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending -// -package io.deephaven.engine.table.impl.util; - -import io.deephaven.chunk.WritableChunk; -import io.deephaven.chunk.WritableLongChunk; -import io.deephaven.engine.rowset.RowSequence; -import io.deephaven.engine.rowset.chunkattributes.RowKeys; -import io.deephaven.engine.table.ChunkSource; -import io.deephaven.engine.table.SharedContext; -import org.jetbrains.annotations.NotNull; - -/** - * A row redirection which implements the identity transformation. - */ -public class IdentityRowRedirection implements RowRedirection { - public static IdentityRowRedirection INSTANCE = new IdentityRowRedirection(); - - // static use only - private IdentityRowRedirection() {} - - @Override - public long get(long outerRowKey) { - return outerRowKey; - } - - @Override - public long getPrev(long outerRowKey) { - return outerRowKey; - } - - @Override - public ChunkSource.FillContext makeFillContext(final int chunkCapacity, final SharedContext sharedContext) { - return new FillContext() { - @Override - public boolean supportsUnboundedFill() { - return true; - } - }; - } - - @Override - public void fillChunk(@NotNull final ChunkSource.FillContext fillContext, - @NotNull final WritableChunk innerRowKeys, - @NotNull final RowSequence outerRowKeys) { - final WritableLongChunk innerRowKeysTyped = innerRowKeys.asWritableLongChunk(); - innerRowKeysTyped.setSize(outerRowKeys.intSize()); - outerRowKeys.fillRowKeyChunk(innerRowKeysTyped); - } - - @Override - public void fillPrevChunk(@NotNull final ChunkSource.FillContext fillContext, - @NotNull final WritableChunk innerRowKeys, - @NotNull final RowSequence outerRowKeys) { - fillChunk(fillContext, innerRowKeys, outerRowKeys); - } - - @Override - public boolean ascendingMapping() { - return true; - } - - - @Override - public String toString() { - return getClass().getSimpleName(); - } -} From d9216698c6cda23493b5d8d3eed15bded0853a88 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 16:26:38 -0500 Subject: [PATCH 06/14] spotless, from hell's heart I stab at thee. --- .../main/java/io/deephaven/engine/table/impl/SortOperation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index 97923bad984..ebf697cdd31 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -348,7 +348,7 @@ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) return null; } - return ((RedirectedColumnSource)sortResult.getColumnSource((String)columnName)).getRowRedirection(); + return ((RedirectedColumnSource) sortResult.getColumnSource((String) columnName)).getRowRedirection(); } /** From ce77f78b9f52871aa24a4cab1f13e6db8bce5019 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 17:21:14 -0500 Subject: [PATCH 07/14] go expects --- go/pkg/client/example_import_table_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/pkg/client/example_import_table_test.go b/go/pkg/client/example_import_table_test.go index 13a549e3be8..96ad85ebb00 100644 --- a/go/pkg/client/example_import_table_test.go +++ b/go/pkg/client/example_import_table_test.go @@ -93,7 +93,7 @@ func Example_importTable() { // metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "float"] // - Volume: type=int32, nullable // metadata: ["deephaven:isDateFormat": "false", "deephaven:isNumberFormat": "false", "deephaven:isPartitioning": "false", "deephaven:isRowStyle": "false", "deephaven:isSortable": "true", "deephaven:isStyle": "false", "deephaven:type": "int"] - // 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", "deephaven:unsent.attribute.SortRowRedirection": ""] + // 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"] // rows: 5 // col[0][Ticker]: ["IBM" "XRX" "XYZZY" "GME" "ZNGA"] // col[1][Close]: [38.7 53.8 88.5 453 544.9] From c66343e426652606eb0cdc6ae9413db5a488e9c6 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 18:03:00 -0500 Subject: [PATCH 08/14] stupid big timeouts --- .../web/client/api/subscription/ViewportTestGwt.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java index ff94060016c..31223827154 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java @@ -97,7 +97,7 @@ public void testChangePendingViewport() { connect(tables) .then(table("staticTable")) .then(table -> { - delayTestFinish(5001); + delayTestFinish(60001); table.setViewport(0, 25, null); assertEquals(100.0, table.getSize()); @@ -174,7 +174,7 @@ public void testViewportOnUpdatingTable() { connect(tables) .then(table("growingBackward")) .then(table -> { - delayTestFinish(4000); + delayTestFinish(60002); // set up a viewport, and watch it show up, and tick once table.setViewport(0, 9, null); return assertUpdateReceived(table, viewportData -> { From c4c419ca2cb2775b58631681c23793eed30a769a Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Wed, 12 Feb 2025 18:55:55 -0500 Subject: [PATCH 09/14] cleanup the string a little bit --- .../io/deephaven/engine/table/impl/SortOperation.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index ebf697cdd31..e469b25b921 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -34,7 +34,8 @@ public class SortOperation implements QueryTable.MemoizableOperation { static final Map IDENTITY_REDIRECTION_ATTRIBUTES; - static final Object IDENTITY_REDIRECTION_VALUE = new String(); + // The "+" sign is not valid in a column, therefore we can be sure that this is a proper sentinel value. + static final String IDENTITY_REDIRECTION_VALUE = "+IDENTITY_REDIRECTION"; static { final HashMap identityRedirectionAttributes = new HashMap<>(); identityRedirectionAttributes.put(SORT_ROW_REDIRECTION_ATTRIBUTE, IDENTITY_REDIRECTION_VALUE); @@ -343,12 +344,12 @@ public Result initialize(boolean usePrev, long beforeClock) { * @return The row redirection for this table. */ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) { - final Object columnName = sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); - if (columnName == null || columnName == IDENTITY_REDIRECTION_VALUE) { + final String columnName = (String)sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); + if (columnName == null || columnName.equals(IDENTITY_REDIRECTION_VALUE)) { return null; } - return ((RedirectedColumnSource) sortResult.getColumnSource((String) columnName)).getRowRedirection(); + return ((RedirectedColumnSource) sortResult.getColumnSource(columnName)).getRowRedirection(); } /** From 49f7779767a8f582b4c6cd3c15136007486a472d Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 13 Feb 2025 07:26:28 -0500 Subject: [PATCH 10/14] Revert "stupid big timeouts" This reverts commit c66343e426652606eb0cdc6ae9413db5a488e9c6. --- .../web/client/api/subscription/ViewportTestGwt.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java index 31223827154..ff94060016c 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/subscription/ViewportTestGwt.java @@ -97,7 +97,7 @@ public void testChangePendingViewport() { connect(tables) .then(table("staticTable")) .then(table -> { - delayTestFinish(60001); + delayTestFinish(5001); table.setViewport(0, 25, null); assertEquals(100.0, table.getSize()); @@ -174,7 +174,7 @@ public void testViewportOnUpdatingTable() { connect(tables) .then(table("growingBackward")) .then(table -> { - delayTestFinish(60002); + delayTestFinish(4000); // set up a viewport, and watch it show up, and tick once table.setViewport(0, 9, null); return assertUpdateReceived(table, viewportData -> { From e83361a89ee4f36d9c4904efbdb85e70d1a75d0b Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 13 Feb 2025 07:35:25 -0500 Subject: [PATCH 11/14] refactor common code. --- .../engine/table/impl/SortOperation.java | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index e469b25b921..4bb5945b516 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -138,21 +138,12 @@ private QueryTable historicalSort(SortHelpers.SortMapping sortedKeys) { return withSorted(parent); } - // if nothing is actually redirected, we can use the identity value - Object sortMappingColumnName = IDENTITY_REDIRECTION_VALUE; final WritableRowRedirection sortMapping = sortedKeys.makeHistoricalRowRedirection(); final TrackingRowSet resultRowSet = RowSetFactory.flat(sortedKeys.size()).toTracking(); final Map> resultMap = new LinkedHashMap<>(); - for (Map.Entry> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) { - final ColumnSource innerSource = stringColumnSourceEntry.getValue(); - ColumnSource redirectedSource = RedirectedColumnSource.maybeRedirect(sortMapping, innerSource); - resultMap.put(stringColumnSourceEntry.getKey(), redirectedSource); - if (redirectedSource != innerSource) { - sortMappingColumnName = stringColumnSourceEntry.getKey(); - } - } + final String sortMappingColumnName = populateRedirectedColumns(resultMap, sortMapping); resultTable = new QueryTable(resultRowSet, resultMap); parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort); @@ -296,10 +287,7 @@ public Result initialize(boolean usePrev, long beforeClock) { sortMapping.writableCast().fillFromChunk(fillFromContext, LongChunk.chunkWrap(sortedKeys), closer.add(resultRowSet.copy())); - for (Map.Entry> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) { - resultMap.put(stringColumnSourceEntry.getKey(), - RedirectedColumnSource.maybeRedirect(sortMapping, stringColumnSourceEntry.getValue())); - } + String sortMappingColumnName = populateRedirectedColumns(resultMap, sortMapping); // noinspection unchecked final ColumnSource>[] sortedColumnsToSortBy = @@ -314,7 +302,7 @@ public Result initialize(boolean usePrev, long beforeClock) { resultTable = new QueryTable(resultRowSet, resultMap); parent.copyAttributes(resultTable, BaseTable.CopyAttributeOperation.Sort); - resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMapping); + resultTable.setAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE, sortMappingColumnName); setReverseLookup(resultTable, (final long innerRowKey) -> { final long outerRowKey = reverseLookup.get(innerRowKey); return outerRowKey == reverseLookup.getNoEntryValue() ? RowSequence.NULL_ROW_KEY : outerRowKey; @@ -337,6 +325,21 @@ public Result initialize(boolean usePrev, long beforeClock) { } } + private String populateRedirectedColumns(Map> resultMap, RowRedirection sortMapping) { + // if nothing is actually redirected, we can use the identity value + String sortMappingColumnName = IDENTITY_REDIRECTION_VALUE; + + for (Map.Entry> stringColumnSourceEntry : parent.getColumnSourceMap().entrySet()) { + final ColumnSource innerSource = stringColumnSourceEntry.getValue(); + final ColumnSource redirectedSource = RedirectedColumnSource.maybeRedirect(sortMapping, innerSource); + resultMap.put(stringColumnSourceEntry.getKey(), redirectedSource); + if (redirectedSource != innerSource) { + sortMappingColumnName = stringColumnSourceEntry.getKey(); + } + } + return sortMappingColumnName; + } + /** * Get the row redirection for a sort result. * @@ -344,7 +347,7 @@ public Result initialize(boolean usePrev, long beforeClock) { * @return The row redirection for this table. */ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) { - final String columnName = (String)sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); + final String columnName = (String) sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); if (columnName == null || columnName.equals(IDENTITY_REDIRECTION_VALUE)) { return null; } From e46828a985a7a2df67940b604abd1107c05da213 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 13 Feb 2025 08:36:04 -0500 Subject: [PATCH 12/14] javadoc tweaks. --- .../java/io/deephaven/engine/table/impl/SortOperation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index 4bb5945b516..882f0f3c35a 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -344,7 +344,7 @@ private String populateRedirectedColumns(Map> resultMap, * Get the row redirection for a sort result. * * @param sortResult The sort result table; must be the direct result of a sort. - * @return The row redirection for this table. + * @return The row redirection for this table if at least one column required redirection, otherwise {@code null} */ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) { final String columnName = (String) sortResult.getAttribute(SORT_ROW_REDIRECTION_ATTRIBUTE); @@ -371,7 +371,7 @@ public static RowRedirection getRowRedirection(@NotNull final Table sortResult) * * @param parent The sort input table; must have been sorted in order to produce {@code sortResult} * @param sortResult The sort result table; must be the direct result of a sort on {@code parent} - * @return The reverse lookup + * @return The reverse lookup, or null if no redirection is performed. */ public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @NotNull final Table sortResult) { if (BlinkTableTools.isBlink(parent)) { From e76c377042478a05a6cd06d4afd7aef12ad88df2 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 13 Feb 2025 13:34:50 -0500 Subject: [PATCH 13/14] Update engine/api/src/main/java/io/deephaven/engine/table/Table.java Co-authored-by: Larry Booker --- engine/api/src/main/java/io/deephaven/engine/table/Table.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/api/src/main/java/io/deephaven/engine/table/Table.java b/engine/api/src/main/java/io/deephaven/engine/table/Table.java index 435ab1d89ba..7a8ad5be5a2 100644 --- a/engine/api/src/main/java/io/deephaven/engine/table/Table.java +++ b/engine/api/src/main/java/io/deephaven/engine/table/Table.java @@ -185,7 +185,7 @@ public interface Table extends */ String SORT_REVERSE_LOOKUP_ATTRIBUTE = "SortReverseLookup"; /** - * Attribute on sort results used for hierarchical table construction. Specificaiton is left to the implementation. + * Attribute on sort results used for hierarchical table construction. Specification is left to the implementation. */ String SORT_ROW_REDIRECTION_ATTRIBUTE = "SortRowRedirection"; From 70203b408ab63cdb69f4b2aea78c6a395542486c Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 13 Feb 2025 13:35:38 -0500 Subject: [PATCH 14/14] Newline --- .../main/java/io/deephaven/engine/table/impl/SortOperation.java | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java index 882f0f3c35a..81706486703 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java @@ -138,7 +138,6 @@ private QueryTable historicalSort(SortHelpers.SortMapping sortedKeys) { return withSorted(parent); } - final WritableRowRedirection sortMapping = sortedKeys.makeHistoricalRowRedirection(); final TrackingRowSet resultRowSet = RowSetFactory.flat(sortedKeys.size()).toTracking();