fix: WouldMatch does not properly memoize. (deephaven#6313)

Fixes deephaven#6312.

Author: cpwright

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 {

engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java

@@ -3841,7 +3841,9 @@ public <R> R apply(@NotNull final Function<Table, R> 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);
         }
     }
 

engine/table/src/main/java/io/deephaven/engine/table/impl/WouldMatchOperation.java

@@ -92,17 +92,30 @@ 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.
                     return filter.beginOperation(parent);
                 }).collect(SafeCloseableList.COLLECTOR);
     }
 
+    /**
+     * Initialize the filters.
+     *
+     * <p>
+     * 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.
+     * </p>
+     *
+     * @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<NotificationQueue.Dependency> dependencies = WhereListener.extractDependencies(whereFilters);
@@ -180,7 +193,8 @@ public Result<QueryTable> initialize(boolean usePrev, long beforeClock) {
 
     @Override
     public MemoizedOperationKey getMemoizedOperationKey() {
-        return MemoizedOperationKey.wouldMatch();
+        return MemoizedOperationKey.wouldMatch(
+                matchColumns.stream().map(ColumnHolder::getColumnName).toArray(String[]::new), whereFilters);
     }
 
     /**

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

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"));

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);
+    }
 }