From f8eedf32e266109050417259438c16199c06773b Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Mon, 30 Oct 2023 18:31:25 -0600 Subject: [PATCH 1/7] Static Select: Re-Use Already Flattened Results --- .../select/analyzers/PreserveColumnLayer.java | 14 ++- .../analyzers/SelectAndViewAnalyzer.java | 57 +++++++++-- .../impl/QueryTableSelectUpdateTest.java | 97 +++++++++++++++++++ 3 files changed, 158 insertions(+), 10 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java index 8fe1841b205..d6bf93c9f13 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java @@ -24,11 +24,13 @@ */ final public class PreserveColumnLayer extends DependencyLayerBase { private final BitSet dependencyBitSet; + private final boolean flattenedResult; PreserveColumnLayer(SelectAndViewAnalyzer inner, String name, SelectColumn sc, ColumnSource cs, String[] deps, - ModifiedColumnSet mcsBuilder) { + ModifiedColumnSet mcsBuilder, boolean flattenedResult) { super(inner, name, sc, cs, deps, mcsBuilder); this.dependencyBitSet = new BitSet(); + this.flattenedResult = flattenedResult; Arrays.stream(deps).mapToInt(inner::getLayerIndexFor).forEach(dependencyBitSet::set); } @@ -75,6 +77,16 @@ public LogOutput append(LogOutput logOutput) { .append("}"); } + @Override + public boolean flattenedResult() { + return flattenedResult; + } + + @Override + public boolean alreadyFlattenedSources() { + // we can only preserve a flat source if it was already made flat + return flattenedResult; + } @Override public boolean allowCrossColumnParallelization() { diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java index eb34118632c..587c9a7038b 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java @@ -5,6 +5,7 @@ import io.deephaven.base.Pair; import io.deephaven.base.log.LogOutputAppendable; +import io.deephaven.base.verify.Assert; import io.deephaven.datastructures.util.CollectionUtil; import io.deephaven.engine.liveness.LivenessNode; import io.deephaven.engine.rowset.RowSet; @@ -100,6 +101,7 @@ public static SelectAndViewAnalyzerWrapper create( boolean shiftColumnHasPositiveOffset = false; final HashSet resultColumns = flattenedResult ? new HashSet<>() : null; + final HashMap> resultAlias = flattenedResult ? new HashMap<>() : null; for (final SelectColumn sc : selectColumns) { if (remainingCols != null) { remainingCols.add(sc); @@ -121,8 +123,12 @@ public static SelectAndViewAnalyzerWrapper create( // we must re-initialize the column inputs as they may have changed post-flatten sc.initInputs(rowSet, analyzer.getAllColumnSources()); - } else if (!flatResult && flattenedResult) { + } + + if (flattenedResult) { resultColumns.add(sc.getName()); + // this shadows any known alias + resultAlias.remove(sc.getName()); } final Stream allDependencies = @@ -156,20 +162,47 @@ public static SelectAndViewAnalyzerWrapper create( continue; } + final SourceColumn realColumn; + if (sc instanceof SourceColumn) { + realColumn = (SourceColumn) sc; + } else if ((sc instanceof SwitchColumn) && ((SwitchColumn) sc).getRealColumn() instanceof SourceColumn) { + realColumn = (SourceColumn) ((SwitchColumn) sc).getRealColumn(); + } else { + realColumn = null; + } + if (shouldPreserve(sc)) { - if (numberOfInternallyFlattenedColumns > 0) { + // this must be a source column to be preserved + Assert.neqNull(realColumn, "realColumn"); + + boolean sourceIsNew = resultColumns != null && resultColumns.contains(realColumn.getSourceName()); + if (!sourceIsNew && numberOfInternallyFlattenedColumns > 0) { // we must preserve this column, but have already created an analyzer for the internally flattened // column, therefore must start over without permitting internal flattening return create(sourceTable, mode, columnSources, originalRowSet, parentMcs, publishTheseSources, - false, selectColumns); + useShiftedColumns, false, selectColumns); + } + + analyzer = analyzer.createLayerForPreserve( + sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder, flatResult && flattenedResult); + + if (!sourceIsNew) { + // we can not flatten future columns because we are preserving this column + flattenedResult = false; } - analyzer = - analyzer.createLayerForPreserve(sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder); - // we can not flatten future columns because we are preserving this column - flattenedResult = false; continue; } + if (flattenedResult && realColumn != null) { + // this could be a duplicate of a previously flattened column + final ColumnSource alias = resultAlias.get(realColumn.getSourceName()); + if (alias != null) { + analyzer = analyzer.createLayerForPreserve( + sc.getName(), sc, alias, distinctDeps, mcsBuilder, flatResult); + continue; + } + } + final long targetDestinationCapacity = rowSet.isEmpty() ? 0 : (flattenedResult ? rowSet.size() : rowSet.lastRowKey() + 1); switch (mode) { @@ -189,6 +222,12 @@ public static SelectAndViewAnalyzerWrapper create( final WritableColumnSource scs = flatResult || flattenedResult ? sc.newFlatDestInstance(targetDestinationCapacity) : sc.newDestInstance(targetDestinationCapacity); + + if (flattenedResult && realColumn != null && !resultColumns.contains(realColumn.getSourceName())) { + // this source column a candidate for preservation if referenced again + resultAlias.put(realColumn.getSourceName(), scs); + } + analyzer = analyzer.createLayerForSelect(updateGraph, rowSet, sc.getName(), sc, scs, null, distinctDeps, mcsBuilder, false, flattenedResult, flatResult && flattenedResult); if (flattenedResult) { @@ -350,8 +389,8 @@ private SelectAndViewAnalyzer createLayerForView(String name, SelectColumn sc, C } private SelectAndViewAnalyzer createLayerForPreserve(String name, SelectColumn sc, ColumnSource cs, - String[] parentColumnDependencies, ModifiedColumnSet mcsBuilder) { - return new PreserveColumnLayer(this, name, sc, cs, parentColumnDependencies, mcsBuilder); + String[] parentColumnDependencies, ModifiedColumnSet mcsBuilder, boolean flatResult) { + return new PreserveColumnLayer(this, name, sc, cs, parentColumnDependencies, mcsBuilder, flatResult); } abstract void populateModifiedColumnSetRecurse(ModifiedColumnSet mcsBuilder, Set remainingDepsToSatisfy); diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java index 7434df60580..8795a9afd79 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java @@ -33,6 +33,7 @@ import io.deephaven.engine.testutil.testcase.RefreshingTableTestCase; import io.deephaven.engine.util.TableTools; import io.deephaven.util.SafeCloseable; +import io.deephaven.vector.LongVector; import junit.framework.TestCase; import org.apache.commons.lang3.mutable.MutableInt; import org.junit.Assert; @@ -1115,4 +1116,100 @@ public void testRegressionGH3562() { assertTableEquals(expected, result); } + + @Test + public void testStaticSelectPreserveAlreadyFlattenedColumns() { + final Table source = emptyTable(10).updateView("I = ii").where("I % 2 == 0"); + final Table result = source.select("Foo = I", "Bar = Foo", "Baz = I"); + + Assert.assertTrue(result.isFlat()); + + final ColumnSource foo = result.getColumnSource("Foo"); + final ColumnSource bar = result.getColumnSource("Bar"); + final ColumnSource baz = result.getColumnSource("Baz"); + result.getRowSet().forAllRowKeys(rowKey -> { + Assert.assertEquals(rowKey * 2, foo.getLong(rowKey)); + Assert.assertEquals(rowKey * 2, bar.getLong(rowKey)); + Assert.assertEquals(rowKey * 2, baz.getLong(rowKey)); + }); + + Assert.assertEquals(foo, bar); + Assert.assertEquals(foo, baz); + } + + @Test + public void testStaticSelectPreserveColumn() { + final Table source = emptyTable(10).select("I = ii").where("I % 2 == 0"); + final Table result = source.select("Foo = I", "Bar = Foo", "Baz = I"); + + Assert.assertFalse(result.isFlat()); + + final ColumnSource orig = source.getColumnSource("I"); + final ColumnSource foo = result.getColumnSource("Foo"); + final ColumnSource bar = result.getColumnSource("Bar"); + final ColumnSource baz = result.getColumnSource("Baz"); + result.getRowSet().forAllRowKeys(rowKey -> { + Assert.assertEquals(rowKey, foo.getLong(rowKey)); + Assert.assertEquals(rowKey, bar.getLong(rowKey)); + Assert.assertEquals(rowKey, baz.getLong(rowKey)); + }); + + // These columns were preserved and no flattening occurred. + Assert.assertEquals(orig, foo); + Assert.assertEquals(orig, bar); + Assert.assertEquals(orig, baz); + } + + @Test + public void testStaticSelectFlattenNotReusedWithRename() { + final Table source = emptyTable(10).updateView("I = ii").where("I % 2 == 0"); + // we must use a vector column to prevent the inner column from being preserved + final Table result = source.select( + "Foo = I", "I = new io.deephaven.vector.LongVectorDirect(0L, 1L)", "Baz = I"); + + Assert.assertTrue(result.isFlat()); + + final ColumnSource orig = source.getColumnSource("I"); + final ColumnSource foo = result.getColumnSource("Foo"); + final ColumnSource newI = result.getColumnSource("I"); + final ColumnSource baz = result.getColumnSource("Baz"); + result.getRowSet().forAllRowKeys(rowKey -> { + Assert.assertEquals(rowKey * 2, foo.getLong(rowKey)); + for (int ii = 0; ii < 2; ++ii) { + Assert.assertEquals(ii, ((LongVector) newI.get(rowKey)).get(ii)); + Assert.assertEquals(ii, ((LongVector) baz.get(rowKey)).get(ii)); + } + }); + + Assert.assertNotEquals(orig, foo); // this column was flattened + Assert.assertNotEquals(newI, baz); // vector columns cannot be preserved; so this should be a copy + } + + @Test + public void testStaticSelectRevertInternalFlatten() { + // there is some special logic that prevents an internal flatten if it also needs to preserve an original column + final Table source = emptyTable(10) + .select("I = ii") + .updateView("J = ii") + .where("I % 2 == 0"); + + // here `Foo` should be flattened, but `Bar` must be preserved; `Baz` is just for fun + final Table result = source.select("Foo = J", "Bar = I", "Baz = Foo"); + + Assert.assertFalse(result.isFlat()); + + final ColumnSource foo = result.getColumnSource("Foo"); + final ColumnSource bar = result.getColumnSource("Bar"); + final ColumnSource baz = result.getColumnSource("Baz"); + result.getRowSet().forAllRowKeys(rowKey -> { + Assert.assertEquals(rowKey, foo.getLong(rowKey)); + Assert.assertEquals(rowKey, bar.getLong(rowKey)); + Assert.assertEquals(rowKey, baz.getLong(rowKey)); + }); + + // Note that Foo is still being "selected" and therefore "brought into memory" + Assert.assertNotEquals(foo, source.getColumnSource("J")); + Assert.assertEquals(bar, source.getColumnSource("I")); + Assert.assertEquals(baz, foo); + } } From eb601af0d7fae40fc600d4349e2e419960cebfb9 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Thu, 2 Nov 2023 12:41:27 -0600 Subject: [PATCH 2/7] Charles' Feedback --- .../analyzers/SelectAndViewAnalyzer.java | 49 +++++++++---------- .../impl/QueryTableSelectUpdateTest.java | 45 +++++++++++++---- 2 files changed, 58 insertions(+), 36 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java index 587c9a7038b..5ef2d9c2570 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java @@ -5,7 +5,6 @@ import io.deephaven.base.Pair; import io.deephaven.base.log.LogOutputAppendable; -import io.deephaven.base.verify.Assert; import io.deephaven.datastructures.util.CollectionUtil; import io.deephaven.engine.liveness.LivenessNode; import io.deephaven.engine.rowset.RowSet; @@ -34,9 +33,12 @@ import java.util.*; import java.util.concurrent.CompletableFuture; +import java.util.function.Consumer; import java.util.stream.Stream; public abstract class SelectAndViewAnalyzer implements LogOutputAppendable { + private static final Consumer> NOOP = ignore -> {}; + public enum Mode { VIEW_LAZY, VIEW_EAGER, SELECT_STATIC, SELECT_REFRESHING, SELECT_REDIRECTED_REFRESHING, SELECT_REDIRECTED_STATIC } @@ -100,8 +102,8 @@ public static SelectAndViewAnalyzerWrapper create( FormulaColumn shiftColumn = null; boolean shiftColumnHasPositiveOffset = false; - final HashSet resultColumns = flattenedResult ? new HashSet<>() : null; - final HashMap> resultAlias = flattenedResult ? new HashMap<>() : null; + final HashSet resultColumns = new HashSet<>(); + final HashMap> resultAlias = new HashMap<>(); for (final SelectColumn sc : selectColumns) { if (remainingCols != null) { remainingCols.add(sc); @@ -125,11 +127,9 @@ public static SelectAndViewAnalyzerWrapper create( sc.initInputs(rowSet, analyzer.getAllColumnSources()); } - if (flattenedResult) { - resultColumns.add(sc.getName()); - // this shadows any known alias - resultAlias.remove(sc.getName()); - } + resultColumns.add(sc.getName()); + // this shadows any known alias + resultAlias.remove(sc.getName()); final Stream allDependencies = Stream.concat(sc.getColumns().stream(), sc.getColumnArrays().stream()); @@ -171,11 +171,8 @@ public static SelectAndViewAnalyzerWrapper create( realColumn = null; } - if (shouldPreserve(sc)) { - // this must be a source column to be preserved - Assert.neqNull(realColumn, "realColumn"); - - boolean sourceIsNew = resultColumns != null && resultColumns.contains(realColumn.getSourceName()); + if (realColumn != null && shouldPreserve(sc)) { + boolean sourceIsNew = resultColumns.contains(realColumn.getSourceName()); if (!sourceIsNew && numberOfInternallyFlattenedColumns > 0) { // we must preserve this column, but have already created an analyzer for the internally flattened // column, therefore must start over without permitting internal flattening @@ -187,14 +184,14 @@ public static SelectAndViewAnalyzerWrapper create( sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder, flatResult && flattenedResult); if (!sourceIsNew) { - // we can not flatten future columns because we are preserving this column + // we can not flatten future columns because we are preserving a column that may not be flat flattenedResult = false; } continue; } - if (flattenedResult && realColumn != null) { - // this could be a duplicate of a previously flattened column + // look for an existing alias that can be preserved instead + if (realColumn != null) { final ColumnSource alias = resultAlias.get(realColumn.getSourceName()); if (alias != null) { analyzer = analyzer.createLayerForPreserve( @@ -203,16 +200,22 @@ public static SelectAndViewAnalyzerWrapper create( } } + // if this is a source column, then results are eligible for aliasing + final Consumer> maybeCreateAlias = realColumn == null ? NOOP + : cs -> resultAlias.put(realColumn.getSourceName(), cs); + final long targetDestinationCapacity = rowSet.isEmpty() ? 0 : (flattenedResult ? rowSet.size() : rowSet.lastRowKey() + 1); switch (mode) { case VIEW_LAZY: { final ColumnSource viewCs = sc.getLazyView(); + maybeCreateAlias.accept(viewCs); analyzer = analyzer.createLayerForView(sc.getName(), sc, viewCs, distinctDeps, mcsBuilder); break; } case VIEW_EAGER: { final ColumnSource viewCs = sc.getDataView(); + maybeCreateAlias.accept(viewCs); analyzer = analyzer.createLayerForView(sc.getName(), sc, viewCs, distinctDeps, mcsBuilder); break; } @@ -222,12 +225,7 @@ public static SelectAndViewAnalyzerWrapper create( final WritableColumnSource scs = flatResult || flattenedResult ? sc.newFlatDestInstance(targetDestinationCapacity) : sc.newDestInstance(targetDestinationCapacity); - - if (flattenedResult && realColumn != null && !resultColumns.contains(realColumn.getSourceName())) { - // this source column a candidate for preservation if referenced again - resultAlias.put(realColumn.getSourceName(), scs); - } - + maybeCreateAlias.accept(scs); analyzer = analyzer.createLayerForSelect(updateGraph, rowSet, sc.getName(), sc, scs, null, distinctDeps, mcsBuilder, false, flattenedResult, flatResult && flattenedResult); if (flattenedResult) { @@ -239,6 +237,7 @@ public static SelectAndViewAnalyzerWrapper create( final WritableColumnSource underlyingSource = sc.newDestInstance(rowSet.size()); final WritableColumnSource scs = WritableRedirectedColumnSource.maybeRedirect( rowRedirection, underlyingSource, rowSet.size()); + maybeCreateAlias.accept(scs); analyzer = analyzer.createLayerForSelect(updateGraph, rowSet, sc.getName(), sc, scs, underlyingSource, distinctDeps, mcsBuilder, true, false, false); break; @@ -255,6 +254,7 @@ public static SelectAndViewAnalyzerWrapper create( scs = WritableRedirectedColumnSource.maybeRedirect( rowRedirection, underlyingSource, rowSet.intSize()); } + maybeCreateAlias.accept(scs); analyzer = analyzer.createLayerForSelect(updateGraph, rowSet, sc.getName(), sc, scs, underlyingSource, distinctDeps, mcsBuilder, rowRedirection != null, false, false); break; @@ -309,10 +309,7 @@ private static boolean hasConstantValue(final SelectColumn sc) { } private static boolean shouldPreserve(final SelectColumn sc) { - if (!(sc instanceof SourceColumn) - && (!(sc instanceof SwitchColumn) || !(((SwitchColumn) sc).getRealColumn() instanceof SourceColumn))) { - return false; - } + // we already know sc is a SourceColumn or switches to a SourceColumn final ColumnSource sccs = sc.getDataView(); return sccs instanceof InMemoryColumnSource && ((InMemoryColumnSource) sccs).isInMemory() && !Vector.class.isAssignableFrom(sc.getReturnedType()); diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java index 8795a9afd79..bf3b1142efe 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableSelectUpdateTest.java @@ -41,6 +41,7 @@ import org.junit.Test; import java.util.*; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import static io.deephaven.engine.testutil.TstUtils.*; @@ -1133,8 +1134,8 @@ public void testStaticSelectPreserveAlreadyFlattenedColumns() { Assert.assertEquals(rowKey * 2, baz.getLong(rowKey)); }); - Assert.assertEquals(foo, bar); - Assert.assertEquals(foo, baz); + Assert.assertSame(foo, bar); + Assert.assertSame(foo, baz); } @Test @@ -1155,9 +1156,9 @@ public void testStaticSelectPreserveColumn() { }); // These columns were preserved and no flattening occurred. - Assert.assertEquals(orig, foo); - Assert.assertEquals(orig, bar); - Assert.assertEquals(orig, baz); + Assert.assertSame(orig, foo); + Assert.assertSame(orig, bar); + Assert.assertSame(orig, baz); } @Test @@ -1181,8 +1182,8 @@ public void testStaticSelectFlattenNotReusedWithRename() { } }); - Assert.assertNotEquals(orig, foo); // this column was flattened - Assert.assertNotEquals(newI, baz); // vector columns cannot be preserved; so this should be a copy + Assert.assertNotSame(orig, foo); // this column was flattened + Assert.assertNotSame(newI, baz); // vector columns cannot be preserved; so this should be a copy } @Test @@ -1208,8 +1209,32 @@ public void testStaticSelectRevertInternalFlatten() { }); // Note that Foo is still being "selected" and therefore "brought into memory" - Assert.assertNotEquals(foo, source.getColumnSource("J")); - Assert.assertEquals(bar, source.getColumnSource("I")); - Assert.assertEquals(baz, foo); + Assert.assertNotSame(foo, source.getColumnSource("J")); + Assert.assertSame(bar, source.getColumnSource("I")); + Assert.assertSame(baz, foo); + } + + @Test + public void testAliasColumnSelectRefreshing() { + final long size = 100; + final AtomicInteger numCalls = new AtomicInteger(); + QueryScope.addParam("counter", numCalls); + final QueryTable source = testRefreshingTable(RowSetFactory.flat(size).toTracking()); + final Table result = source.update("id = counter.getAndIncrement()") + .select("id_a = id", "id_b = id"); + + final ColumnSource id_a = result.getColumnSource("id_a"); + final ColumnSource id_b = result.getColumnSource("id_b"); + Assert.assertSame(id_a, id_b); + Assert.assertEquals(numCalls.intValue(), size); + + final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); + updateGraph.runWithinUnitTestCycle(() -> { + final WritableRowSet added = RowSetFactory.fromRange(size, size * 2 - 1); + addToTable(source, added); + source.notifyListeners(added, i(), i()); + }); + + Assert.assertEquals(numCalls.intValue(), 2 * size); } } From 3e642ab8a2b4456d2e3f8aaf6ef633b3c7e1293d Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Thu, 2 Nov 2023 12:45:24 -0600 Subject: [PATCH 3/7] apply spotless --- .../table/impl/select/analyzers/SelectAndViewAnalyzer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java index 5ef2d9c2570..20bc7928a49 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java @@ -37,7 +37,8 @@ import java.util.stream.Stream; public abstract class SelectAndViewAnalyzer implements LogOutputAppendable { - private static final Consumer> NOOP = ignore -> {}; + private static final Consumer> NOOP = ignore -> { + }; public enum Mode { VIEW_LAZY, VIEW_EAGER, SELECT_STATIC, SELECT_REFRESHING, SELECT_REDIRECTED_REFRESHING, SELECT_REDIRECTED_STATIC From ecdf5eb5a9313ed28a8cc9e08090a40aa61d31bb Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Thu, 2 Nov 2023 17:06:46 -0600 Subject: [PATCH 4/7] Bugfix: reuse original rowset if no flattening required --- .../main/java/io/deephaven/engine/table/impl/QueryTable.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java index 1b468be1e76..3c5e837610c 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java @@ -1525,8 +1525,9 @@ this, mode, columns, rowSet, getModifiedColumnSetForUpdates(), publishTheseSourc } } - final TrackingRowSet resultRowSet = - analyzer.flattenedResult() ? RowSetFactory.flat(rowSet.size()).toTracking() : rowSet; + final TrackingRowSet resultRowSet = analyzer.flattenedResult() && !rowSet.isFlat() + ? RowSetFactory.flat(rowSet.size()).toTracking() + : rowSet; resultTable = new QueryTable(resultRowSet, analyzerWrapper.getPublishedColumnResources()); if (liveResultCapture != null) { analyzer.startTrackingPrev(); From aeb247833423116b1c38e99f7924bda9bcb8e766 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 3 Nov 2023 08:53:21 -0600 Subject: [PATCH 5/7] Per Slack w/Charles --- .../io/deephaven/engine/table/impl/QueryTable.java | 5 ++--- .../impl/select/analyzers/PreserveColumnLayer.java | 11 +++++------ .../impl/select/analyzers/SelectAndViewAnalyzer.java | 9 ++++----- .../impl/select/analyzers/StaticFlattenLayer.java | 12 ++++++++++++ 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java index 3c5e837610c..1b468be1e76 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java @@ -1525,9 +1525,8 @@ this, mode, columns, rowSet, getModifiedColumnSetForUpdates(), publishTheseSourc } } - final TrackingRowSet resultRowSet = analyzer.flattenedResult() && !rowSet.isFlat() - ? RowSetFactory.flat(rowSet.size()).toTracking() - : rowSet; + final TrackingRowSet resultRowSet = + analyzer.flattenedResult() ? RowSetFactory.flat(rowSet.size()).toTracking() : rowSet; resultTable = new QueryTable(resultRowSet, analyzerWrapper.getPublishedColumnResources()); if (liveResultCapture != null) { analyzer.startTrackingPrev(); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java index d6bf93c9f13..6364cde5489 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java @@ -24,13 +24,11 @@ */ final public class PreserveColumnLayer extends DependencyLayerBase { private final BitSet dependencyBitSet; - private final boolean flattenedResult; PreserveColumnLayer(SelectAndViewAnalyzer inner, String name, SelectColumn sc, ColumnSource cs, String[] deps, - ModifiedColumnSet mcsBuilder, boolean flattenedResult) { + ModifiedColumnSet mcsBuilder) { super(inner, name, sc, cs, deps, mcsBuilder); this.dependencyBitSet = new BitSet(); - this.flattenedResult = flattenedResult; Arrays.stream(deps).mapToInt(inner::getLayerIndexFor).forEach(dependencyBitSet::set); } @@ -79,13 +77,14 @@ public LogOutput append(LogOutput logOutput) { @Override public boolean flattenedResult() { - return flattenedResult; + // a preserve layer is only flattened if the inner is flattened + return inner.flattenedResult(); } @Override public boolean alreadyFlattenedSources() { - // we can only preserve a flat source if it was already made flat - return flattenedResult; + // a preserve layer is only already flattened if the inner is already flattened + return inner.alreadyFlattenedSources(); } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java index 20bc7928a49..15e5526637e 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java @@ -182,7 +182,7 @@ public static SelectAndViewAnalyzerWrapper create( } analyzer = analyzer.createLayerForPreserve( - sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder, flatResult && flattenedResult); + sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder); if (!sourceIsNew) { // we can not flatten future columns because we are preserving a column that may not be flat @@ -195,8 +195,7 @@ public static SelectAndViewAnalyzerWrapper create( if (realColumn != null) { final ColumnSource alias = resultAlias.get(realColumn.getSourceName()); if (alias != null) { - analyzer = analyzer.createLayerForPreserve( - sc.getName(), sc, alias, distinctDeps, mcsBuilder, flatResult); + analyzer = analyzer.createLayerForPreserve(sc.getName(), sc, alias, distinctDeps, mcsBuilder); continue; } } @@ -387,8 +386,8 @@ private SelectAndViewAnalyzer createLayerForView(String name, SelectColumn sc, C } private SelectAndViewAnalyzer createLayerForPreserve(String name, SelectColumn sc, ColumnSource cs, - String[] parentColumnDependencies, ModifiedColumnSet mcsBuilder, boolean flatResult) { - return new PreserveColumnLayer(this, name, sc, cs, parentColumnDependencies, mcsBuilder, flatResult); + String[] parentColumnDependencies, ModifiedColumnSet mcsBuilder) { + return new PreserveColumnLayer(this, name, sc, cs, parentColumnDependencies, mcsBuilder); } abstract void populateModifiedColumnSetRecurse(ModifiedColumnSet mcsBuilder, Set remainingDepsToSatisfy); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/StaticFlattenLayer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/StaticFlattenLayer.java index 71ff4317920..1ed084934df 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/StaticFlattenLayer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/StaticFlattenLayer.java @@ -136,4 +136,16 @@ public LogOutput append(LogOutput logOutput) { public boolean allowCrossColumnParallelization() { return inner.allowCrossColumnParallelization(); } + + @Override + public boolean flattenedResult() { + // this layer performs a flatten, so the result is flattened + return true; + } + + @Override + public boolean alreadyFlattenedSources() { + // this layer performs a flatten, so the sources are now flattened + return true; + } } From ac393d1621db385c8f7166ea785929bbd60e8135 Mon Sep 17 00:00:00 2001 From: Nate Bauernfeind Date: Mon, 6 Nov 2023 10:08:06 -0700 Subject: [PATCH 6/7] Charles' comment improvement. Co-authored-by: Charles P. Wright --- .../table/impl/select/analyzers/PreserveColumnLayer.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java index 6364cde5489..5a9247919da 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java @@ -78,6 +78,9 @@ public LogOutput append(LogOutput logOutput) { @Override public boolean flattenedResult() { // a preserve layer is only flattened if the inner is flattened + // the "flattenedResult" means that we are flattening the table as part of select. For a pre-existing column, we + // could not preserve a layer while flattening, but if we are preserving a newly generated column; it is valid for + // the result to have been flattened as part of select. return inner.flattenedResult(); } From e48fd721f224b7edfff9b5afd3c190f4f8a3916c Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Mon, 6 Nov 2023 10:11:50 -0700 Subject: [PATCH 7/7] Spotless + Charles' Feedback --- .../select/analyzers/PreserveColumnLayer.java | 8 ++++---- .../analyzers/SelectAndViewAnalyzer.java | 20 ++++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java index 5a9247919da..533a9c5e850 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/PreserveColumnLayer.java @@ -77,10 +77,10 @@ public LogOutput append(LogOutput logOutput) { @Override public boolean flattenedResult() { - // a preserve layer is only flattened if the inner is flattened - // the "flattenedResult" means that we are flattening the table as part of select. For a pre-existing column, we - // could not preserve a layer while flattening, but if we are preserving a newly generated column; it is valid for - // the result to have been flattened as part of select. + // preserve layer is only flattened if the inner is flattened + // the "flattenedResult" means that we are flattening the table as part of select. For a pre-existing column, we + // could not preserve a layer while flattening, but if we are preserving a newly generated column; it is valid + // for the result to have been flattened as part of select. return inner.flattenedResult(); } diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java index 15e5526637e..d2972981e17 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/analyzers/SelectAndViewAnalyzer.java @@ -174,20 +174,22 @@ public static SelectAndViewAnalyzerWrapper create( if (realColumn != null && shouldPreserve(sc)) { boolean sourceIsNew = resultColumns.contains(realColumn.getSourceName()); - if (!sourceIsNew && numberOfInternallyFlattenedColumns > 0) { - // we must preserve this column, but have already created an analyzer for the internally flattened - // column, therefore must start over without permitting internal flattening - return create(sourceTable, mode, columnSources, originalRowSet, parentMcs, publishTheseSources, - useShiftedColumns, false, selectColumns); + if (!sourceIsNew) { + if (numberOfInternallyFlattenedColumns > 0) { + // we must preserve this column, but have already created an analyzer for the internally + // flattened + // column, therefore must start over without permitting internal flattening + return create(sourceTable, mode, columnSources, originalRowSet, parentMcs, publishTheseSources, + useShiftedColumns, false, selectColumns); + } else { + // we can not flatten future columns because we are preserving a column that may not be flat + flattenedResult = false; + } } analyzer = analyzer.createLayerForPreserve( sc.getName(), sc, sc.getDataView(), distinctDeps, mcsBuilder); - if (!sourceIsNew) { - // we can not flatten future columns because we are preserving a column that may not be flat - flattenedResult = false; - } continue; }