From 6992036dff14e353ce87ebf583135492c3f9e4d0 Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 31 Oct 2024 17:40:17 -0400 Subject: [PATCH] fix: WouldMatch does not properly memoize. (#6313) Fixes #6312. --- .../table/impl/MemoizedOperationKey.java | 20 ++++++---- .../engine/table/impl/QueryTable.java | 4 +- .../table/impl/WouldMatchOperation.java | 24 ++++++++--- .../engine/table/impl/select/MatchFilter.java | 39 +++++++++++++----- .../engine/table/impl/QueryTableTest.java | 5 +++ .../table/impl/QueryTableWhereTest.java | 40 +++++++++++++++++++ 6 files changed, 109 insertions(+), 23 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java index a6f3f272b74..df8244a2d68 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java @@ -8,7 +8,6 @@ import io.deephaven.api.RangeJoinMatch; import io.deephaven.api.agg.Aggregation; import io.deephaven.engine.table.Table; -import io.deephaven.engine.table.WouldMatchPair; import io.deephaven.engine.table.impl.select.*; import io.deephaven.engine.table.impl.sources.regioned.SymbolTableSource; import org.jetbrains.annotations.NotNull; @@ -525,10 +524,12 @@ public int hashCode() { } private static final class WouldMatch extends AttributeAgnosticMemoizedOperationKey { - private final WouldMatchPair[] pairs; + private final String[] names; + private final WhereFilter[] filters; - private WouldMatch(WouldMatchPair[] pairs) { - this.pairs = pairs; + private WouldMatch(String[] names, WhereFilter[] filters) { + this.names = names; + this.filters = filters; } @Override @@ -540,12 +541,12 @@ public boolean equals(final Object other) { return false; } final WouldMatch wouldMatch = (WouldMatch) other; - return Arrays.equals(pairs, wouldMatch.pairs); + return Arrays.equals(names, wouldMatch.names) && Arrays.equals(filters, wouldMatch.filters); } @Override public int hashCode() { - return Arrays.hashCode(pairs); + return Arrays.hashCode(names) ^ Arrays.hashCode(filters); } @Override @@ -554,8 +555,11 @@ BaseTable.CopyAttributeOperation copyType() { } } - public static MemoizedOperationKey wouldMatch(WouldMatchPair... pairs) { - return new WouldMatch(pairs); + public static MemoizedOperationKey wouldMatch(String[] names, WhereFilter... filters) { + if (Arrays.stream(filters).allMatch(WhereFilter::canMemoize)) { + return new WouldMatch(names, filters); + } + return null; } private static class CrossJoin extends AttributeAgnosticMemoizedOperationKey { 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 654e369d496..acb8e764e29 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 @@ -3841,7 +3841,9 @@ public R apply(@NotNull final Function function) { public Table wouldMatch(WouldMatchPair... matchers) { final UpdateGraph updateGraph = getUpdateGraph(); try (final SafeCloseable ignored = ExecutionContext.getContext().withUpdateGraph(updateGraph).open()) { - return getResult(new WouldMatchOperation(this, matchers)); + final WouldMatchOperation operation = new WouldMatchOperation(this, matchers); + operation.initializeFilters(this); + return getResult(operation); } } diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java index eae78588441..43e20c8d9b1 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java @@ -92,10 +92,6 @@ public String getLogPrefix() { @Override public SafeCloseable beginOperation(@NotNull final QueryTable parent) { - final QueryCompilerRequestProcessor.BatchProcessor compilationProcessor = QueryCompilerRequestProcessor.batch(); - Arrays.stream(whereFilters).forEach(filter -> filter.init(parent.getDefinition(), compilationProcessor)); - compilationProcessor.compile(); - return Arrays.stream(whereFilters) .map((final WhereFilter filter) -> { // Ensure we gather the correct dependencies when building a snapshot control. @@ -103,6 +99,23 @@ public SafeCloseable beginOperation(@NotNull final QueryTable parent) { }).collect(SafeCloseableList.COLLECTOR); } + /** + * Initialize the filters. + * + *

+ * We must initialize our filters before the wouldMatch operation's call to QueryTable's getResult method, so that + * memoization processing can correctly compare them. MatchFilters do not properly implement memoization before + * initialization, and they are the most common filter to memoize. + *

+ * + * @param parent the parent table to have wouldMatch applied + */ + void initializeFilters(@NotNull QueryTable parent) { + final QueryCompilerRequestProcessor.BatchProcessor compilationProcessor = QueryCompilerRequestProcessor.batch(); + Arrays.stream(whereFilters).forEach(filter -> filter.init(parent.getDefinition(), compilationProcessor)); + compilationProcessor.compile(); + } + @Override public OperationSnapshotControl newSnapshotControl(@NotNull final QueryTable queryTable) { final List dependencies = WhereListener.extractDependencies(whereFilters); @@ -180,7 +193,8 @@ public Result initialize(boolean usePrev, long beforeClock) { @Override public MemoizedOperationKey getMemoizedOperationKey() { - return MemoizedOperationKey.wouldMatch(); + return MemoizedOperationKey.wouldMatch( + matchColumns.stream().map(ColumnHolder::getColumnName).toArray(String[]::new), whereFilters); } /** diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/MatchFilter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/MatchFilter.java index 4a7921e39b1..9d7353758b0 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/MatchFilter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/MatchFilter.java @@ -854,30 +854,51 @@ private String toString(Object[] x) { @Override public boolean equals(Object o) { - if (this == o) + if (this == o) { return true; - if (o == null || getClass() != o.getClass()) + } + if (o == null || getClass() != o.getClass()) { return false; + } + final MatchFilter that = (MatchFilter) o; - return invertMatch == that.invertMatch && - caseInsensitive == that.caseInsensitive && - Objects.equals(columnName, that.columnName) && - Arrays.equals(values, that.values) && - Arrays.equals(strValues, that.strValues); + + // The equality check is used for memoization, and we cannot actually determine equality of an uninitialized + // filter, because there is too much state that has not been realized. + if (!initialized && !that.initialized) { + throw new UnsupportedOperationException("MatchFilter has not been initialized"); + } + + // start off with the simple things + if (invertMatch != that.invertMatch || + caseInsensitive != that.caseInsensitive || + !Objects.equals(columnName, that.columnName)) { + return false; + } + + if (!Arrays.equals(values, that.values)) { + return false; + } + + return Objects.equals(getFailoverFilterIfCached(), that.getFailoverFilterIfCached()); } @Override public int hashCode() { + if (!initialized) { + throw new UnsupportedOperationException("MatchFilter has not been initialized"); + } int result = Objects.hash(columnName, invertMatch, caseInsensitive); + // we can use values because we know the filter has been initialized; the hash code should be stable and it + // cannot be stable before we convert the values result = 31 * result + Arrays.hashCode(values); - result = 31 * result + Arrays.hashCode(strValues); return result; } @Override public boolean canMemoize() { // we can be memoized once our values have been initialized; but not before - return initialized; + return initialized && (getFailoverFilterIfCached() == null || getFailoverFilterIfCached().canMemoize()); } @Override diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java index 1e81065b84d..5c532df1a65 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java @@ -3280,6 +3280,11 @@ public void testMemoize() { testNoMemoize(source, t -> t.where("Sym in `aa`, `bb`"), t -> t.where("Sym in `aa`, `cc`")); testNoMemoize(source, t -> t.where("Sym.startsWith(`a`)")); + testMemoize(source, t -> t.wouldMatch("A=intCol == 7"), t -> t.wouldMatch("A=intCol == 7")); + testNoMemoize(source, t -> t.wouldMatch("A=intCol == 7"), t -> t.wouldMatch("A=intCol == 6")); + testNoMemoize(source, t -> t.wouldMatch("A=intCol == 7"), t -> t.wouldMatch("B=intCol == 7")); + testNoMemoize(source, t -> t.wouldMatch("A=intCol < 7"), t -> t.wouldMatch("A=intCol < 7")); + testMemoize(source, t -> t.countBy("Count", "Sym")); testMemoize(source, t -> t.countBy("Sym")); testMemoize(source, t -> t.sumBy("Sym")); diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableWhereTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableWhereTest.java index 8b581fd4fce..1dc18266e46 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableWhereTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableWhereTest.java @@ -1677,4 +1677,44 @@ public void testAddAndRemoveRefilter() { assertTableEquals(source.where("FV in `A`, `B`"), result); } + + @Test + public void testWhereFilterEquality() { + final Table x = TableTools.newTable(intCol("A", 1, 2, 3), intCol("B", 4, 2, 1), stringCol("S", "A", "B", "C")); + + final WhereFilter f1 = WhereFilterFactory.getExpression("A in 7"); + final WhereFilter f2 = WhereFilterFactory.getExpression("A in 8"); + final WhereFilter f3 = WhereFilterFactory.getExpression("A in 7"); + + final Table ignored = x.where(Filter.and(f1, f2, f3)); + + assertEquals(f1, f3); + assertNotEquals(f1, f2); + assertNotEquals(f2, f3); + + final WhereFilter fa = WhereFilterFactory.getExpression("A in 7"); + final WhereFilter fb = WhereFilterFactory.getExpression("B in 7"); + final WhereFilter fap = WhereFilterFactory.getExpression("A not in 7"); + + final Table ignored2 = x.where(Filter.and(fa, fb, fap)); + + assertNotEquals(fa, fb); + assertNotEquals(fa, fap); + assertNotEquals(fb, fap); + + final WhereFilter fs = WhereFilterFactory.getExpression("S icase in `A`"); + final WhereFilter fs2 = WhereFilterFactory.getExpression("S icase in `A`, `B`, `C`"); + final WhereFilter fs3 = WhereFilterFactory.getExpression("S icase in `A`, `B`, `C`"); + final Table ignored3 = x.where(Filter.and(fs, fs2, fs3)); + assertNotEquals(fs, fs2); + assertNotEquals(fs, fs3); + assertEquals(fs2, fs3); + + final WhereFilter fof1 = WhereFilterFactory.getExpression("A = B"); + final WhereFilter fof2 = WhereFilterFactory.getExpression("A = B"); + final Table ignored4 = x.where(fof1); + final Table ignored5 = x.where(fof2); + // the ConditionFilters do not compare as equal, so this is unfortunate, but expected behavior + assertNotEquals(fof1, fof2); + } }