Fix WhereFilterAdapter for String literals (#5408)

It was not obvious during the initial implementation, but `RangeConditionFilter` expects String literals to be wrapped in quotes (as it primarily is sourced from `WhereFilterFactory` / query parsing logic). It was also discovered that one of the `MatchFilter` constructors is very non-obvious, and can lead to case-insensitive String matching. This was the case not only in `WhereFilterAdapter`, but also in `PartitionedTable#constituentFor`. The `MatchFilter` constructor has been marked as deprecated, and all internal usages of it have been migrated explicitly. This may be a good motivator to implement #3730.

Fixes #5407

engine/benchmark/src/benchmark/java/io/deephaven/benchmark/engine/MatchFilterBenchmark.java

@@ -6,6 +6,7 @@
 import io.deephaven.engine.context.ExecutionContext;
 import io.deephaven.engine.context.TestExecutionContext;
 import io.deephaven.engine.table.Table;
+import io.deephaven.engine.table.impl.select.MatchFilter.MatchType;
 import io.deephaven.engine.testutil.ControlledUpdateGraph;
 import io.deephaven.time.DateTimeUtils;
 import io.deephaven.engine.table.impl.select.*;
@@ -115,7 +116,7 @@ public void setupEnv(BenchmarkParams params) {
                 values.add(ii);
             }
         }
-        matchFilter = new MatchFilter(filterCol, values.toArray());
+        matchFilter = new MatchFilter(MatchType.Regular, filterCol, values.toArray());
     }
 
     @TearDown(Level.Trial)

engine/table/src/main/java/io/deephaven/engine/table/impl/partitioned/PartitionedTableImpl.java

@@ -22,6 +22,7 @@
 import io.deephaven.engine.table.impl.*;
 import io.deephaven.engine.table.impl.remote.ConstructSnapshot;
 import io.deephaven.engine.table.impl.select.MatchFilter;
+import io.deephaven.engine.table.impl.select.MatchFilter.MatchType;
 import io.deephaven.engine.table.impl.select.WhereFilter;
 import io.deephaven.engine.table.impl.sources.NullValueColumnSource;
 import io.deephaven.engine.table.impl.sources.UnionSourceManager;
@@ -458,7 +459,7 @@ public Table constituentFor(@NotNull final Object... keyColumnValues) {
         final List<MatchFilter> filters = new ArrayList<>(numKeys);
         final String[] keyColumnNames = keyColumnNames().toArray(String[]::new);
         for (int kci = 0; kci < numKeys; ++kci) {
-            filters.add(new MatchFilter(keyColumnNames[kci], keyColumnValues[kci]));
+            filters.add(new MatchFilter(MatchType.Regular, keyColumnNames[kci], keyColumnValues[kci]));
         }
         return LivenessScopeStack.computeEnclosed(() -> {
             final Table[] matchingConstituents = filter(filters).snapshotConstituents();

engine/table/src/main/java/io/deephaven/engine/table/impl/partitioned/PartitionedTableProxyImpl.java

@@ -18,6 +18,7 @@
 import io.deephaven.engine.table.impl.MatchPair;
 import io.deephaven.engine.table.impl.*;
 import io.deephaven.engine.table.impl.select.MatchFilter;
+import io.deephaven.engine.table.impl.select.MatchFilter.MatchType;
 import io.deephaven.engine.table.impl.select.SelectColumn;
 import io.deephaven.engine.table.impl.select.SourceColumn;
 import io.deephaven.engine.table.impl.select.WhereFilter;
@@ -339,7 +340,7 @@ private static DependentValidation matchingKeysValidation(
         final Table rhsKeys = rhs.table().updateView(rhsKeyColumnRenames).selectDistinct(lhsKeyColumnNames);
         final Table unionedKeys = TableTools.merge(lhsKeys, rhsKeys);
         final Table countedKeys = unionedKeys.countBy(FOUND_IN.name(), lhs.keyColumnNames());
-        final Table nonMatchingKeys = countedKeys.where(new MatchFilter(FOUND_IN.name(), 1));
+        final Table nonMatchingKeys = countedKeys.where(new MatchFilter(MatchType.Regular, FOUND_IN.name(), 1));
         final Table nonMatchingKeysOnly = nonMatchingKeys.view(lhsKeyColumnNames);
         checkNonMatchingKeys(nonMatchingKeysOnly);
         return new DependentValidation("Matching Partition Keys", nonMatchingKeysOnly,

engine/table/src/main/java/io/deephaven/engine/table/impl/select/MatchFilter.java

@@ -81,6 +81,11 @@ public MatchFilter(
         this(CaseSensitivity.MatchCase, matchType, columnName, null, values);
     }
 
+    /**
+     * @deprecated this method is non-obvious in using IgnoreCase by default. Use
+     *             {@link MatchFilter#MatchFilter(MatchType, String, Object...)} instead.
+     */
+    @Deprecated(forRemoval = true)
     public MatchFilter(
             @NotNull final String columnName,
             @NotNull final Object... values) {
@@ -501,10 +506,12 @@ Object convertStringLiteral(String str) {
 
     @Override
     public String toString() {
-        if (strValues == null) {
-            return columnName + (invertMatch ? " not" : "") + " in " + Arrays.toString(values);
-        }
-        return columnName + (invertMatch ? " not" : "") + " in " + Arrays.toString(strValues);
+        return strValues == null ? toString(values) : toString(strValues);
+    }
+
+    private String toString(Object[] x) {
+        return columnName + (caseInsensitive ? " icase" : "") + (invertMatch ? " not" : "") + " in "
+                + Arrays.toString(x);
     }
 
     @Override

engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilterAdapter.java

@@ -241,17 +241,40 @@ public PreferredLhsColumnRhsVisitor(ColumnName lhs) {
                 this.lhs = Objects.requireNonNull(lhs);
             }
 
-            private WhereFilter matchOrRange(Object rhs) {
+            // The String vs non-String cases are separated out, as it's necessary in the RangeConditionFilter case to
+            // wrap String literals with quotes (as that's what RangeConditionFilter expects wrt parsing). MatchFilter
+            // allows us to pass in the already parsed Object (otherwise, if we were passing strValues we would need to
+            // wrap them)
+
+            private WhereFilter matchOrRange(Object rhsLiteral) {
+                // TODO(deephaven-core#3730): More efficient io.deephaven.api.filter.FilterComparison to RangeFilter
                 switch (preferred.operator()) {
                     case EQUALS:
-                        return new MatchFilter(lhs.name(), rhs);
+                        return new MatchFilter(MatchType.Regular, lhs.name(), rhsLiteral);
                     case NOT_EQUALS:
-                        return new MatchFilter(MatchType.Inverted, lhs.name(), rhs);
+                        return new MatchFilter(MatchType.Inverted, lhs.name(), rhsLiteral);
+                    case LESS_THAN:
+                    case LESS_THAN_OR_EQUAL:
+                    case GREATER_THAN:
+                    case GREATER_THAN_OR_EQUAL:
+                        return range(rhsLiteral);
+                    default:
+                        throw new IllegalStateException("Unexpected operator " + original.operator());
+                }
+            }
+
+            private WhereFilter matchOrRange(String rhsLiteral) {
+                // TODO(deephaven-core#3730): More efficient io.deephaven.api.filter.FilterComparison to RangeFilter
+                switch (preferred.operator()) {
+                    case EQUALS:
+                        return new MatchFilter(MatchType.Regular, lhs.name(), rhsLiteral);
+                    case NOT_EQUALS:
+                        return new MatchFilter(MatchType.Inverted, lhs.name(), rhsLiteral);
                     case LESS_THAN:
                     case LESS_THAN_OR_EQUAL:
                     case GREATER_THAN:
                     case GREATER_THAN_OR_EQUAL:
-                        return range(rhs.toString());
+                        return range(rhsLiteral);
                     default:
                         throw new IllegalStateException("Unexpected operator " + original.operator());
                 }
@@ -307,7 +330,7 @@ public WhereFilter visit(String rhs) {
             public WhereFilter visit(boolean rhs) {
                 switch (preferred.operator()) {
                     case EQUALS:
-                        return new MatchFilter(lhs.name(), rhs);
+                        return new MatchFilter(MatchType.Regular, lhs.name(), rhs);
                     case NOT_EQUALS:
                         return new MatchFilter(MatchType.Inverted, lhs.name(), rhs);
                     case LESS_THAN:
@@ -345,17 +368,34 @@ public WhereFilter visit(RawString rawString) {
                 return original();
             }
 
-            private RangeConditionFilter range(String rhsValue) {
+            private RangeConditionFilter range(Object rhsLiteral) {
+                // TODO(deephaven-core#3730): More efficient io.deephaven.api.filter.FilterComparison to RangeFilter
+                final String rhsLiteralAsStr = rhsLiteral.toString();
+                switch (preferred.operator()) {
+                    case LESS_THAN:
+                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN, rhsLiteralAsStr);
+                    case LESS_THAN_OR_EQUAL:
+                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN_OR_EQUAL, rhsLiteralAsStr);
+                    case GREATER_THAN:
+                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN, rhsLiteralAsStr);
+                    case GREATER_THAN_OR_EQUAL:
+                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN_OR_EQUAL, rhsLiteralAsStr);
+                }
+                throw new IllegalStateException("Unexpected");
+            }
+
+            private RangeConditionFilter range(String rhsLiteral) {
                 // TODO(deephaven-core#3730): More efficient io.deephaven.api.filter.FilterComparison to RangeFilter
+                final String quotedRhsLiteral = '"' + rhsLiteral + '"';
                 switch (preferred.operator()) {
                     case LESS_THAN:
-                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN, rhsValue);
+                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN, quotedRhsLiteral);
                     case LESS_THAN_OR_EQUAL:
-                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN_OR_EQUAL, rhsValue);
+                        return new RangeConditionFilter(lhs.name(), Condition.LESS_THAN_OR_EQUAL, quotedRhsLiteral);
                     case GREATER_THAN:
-                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN, rhsValue);
+                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN, quotedRhsLiteral);
                     case GREATER_THAN_OR_EQUAL:
-                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN_OR_EQUAL, rhsValue);
+                        return new RangeConditionFilter(lhs.name(), Condition.GREATER_THAN_OR_EQUAL, quotedRhsLiteral);
                 }
                 throw new IllegalStateException("Unexpected");
             }

engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilterFactory.java

@@ -319,24 +319,24 @@ private static WhereFilter createQuickFilter(ColumnDefinition<?> colDef, String
             try {
                 return DoubleRangeFilter.makeRange(colName, quickFilter);
             } catch (NumberFormatException ignored) {
-                return new MatchFilter(colName, typeData.doubleVal);
+                return new MatchFilter(MatchType.Regular, colName, typeData.doubleVal);
             }
         } else if (colClass == Float.class || colClass == float.class && (!Float.isNaN(typeData.floatVal))) {
             try {
                 return FloatRangeFilter.makeRange(colName, quickFilter);
             } catch (NumberFormatException ignored) {
-                return new MatchFilter(colName, typeData.floatVal);
+                return new MatchFilter(MatchType.Regular, colName, typeData.floatVal);
             }
         } else if ((colClass == Integer.class || colClass == int.class) && typeData.isInt) {
-            return new MatchFilter(colName, typeData.intVal);
+            return new MatchFilter(MatchType.Regular, colName, typeData.intVal);
         } else if ((colClass == long.class || colClass == Long.class) && typeData.isLong) {
-            return new MatchFilter(colName, typeData.longVal);
+            return new MatchFilter(MatchType.Regular, colName, typeData.longVal);
         } else if ((colClass == short.class || colClass == Short.class) && typeData.isShort) {
-            return new MatchFilter(colName, typeData.shortVal);
+            return new MatchFilter(MatchType.Regular, colName, typeData.shortVal);
         } else if ((colClass == byte.class || colClass == Byte.class) && typeData.isByte) {
-            return new MatchFilter(colName, typeData.byteVal);
+            return new MatchFilter(MatchType.Regular, colName, typeData.byteVal);
         } else if (colClass == BigInteger.class && typeData.isBigInt) {
-            return new MatchFilter(colName, typeData.bigIntVal);
+            return new MatchFilter(MatchType.Regular, colName, typeData.bigIntVal);
         } else if (colClass == BigDecimal.class && typeData.isBigDecimal) {
             return ComparableRangeFilter.makeBigDecimalRange(colName, quickFilter);
         } else if (filterMode != QuickFilterMode.NUMERIC) {
@@ -347,11 +347,11 @@ private static WhereFilter createQuickFilter(ColumnDefinition<?> colDef, String
                         Mode.FIND,
                         false), false);
             } else if ((colClass == boolean.class || colClass == Boolean.class) && typeData.isBool) {
-                return new MatchFilter(colName, Boolean.parseBoolean(quickFilter));
+                return new MatchFilter(MatchType.Regular, colName, Boolean.parseBoolean(quickFilter));
             } else if (colClass == Instant.class && typeData.dateLower != null && typeData.dateUpper != null) {
                 return new InstantRangeFilter(colName, typeData.dateLower, typeData.dateUpper, true, false);
             } else if ((colClass == char.class || colClass == Character.class) && typeData.isChar) {
-                return new MatchFilter(colName, typeData.charVal);
+                return new MatchFilter(MatchType.Regular, colName, typeData.charVal);
             }
         }
         return null;

engine/table/src/test/java/io/deephaven/engine/table/impl/TestPartitionBy.java

@@ -10,6 +10,7 @@
 import io.deephaven.engine.table.PartitionedTable;
 import io.deephaven.engine.table.Table;
 import io.deephaven.engine.table.hierarchical.RollupTable;
+import io.deephaven.engine.table.impl.select.MatchFilter.MatchType;
 import io.deephaven.engine.testutil.*;
 import io.deephaven.engine.testutil.generator.IntGenerator;
 import io.deephaven.engine.testutil.generator.SetGenerator;
@@ -78,11 +79,11 @@ public void validate(String msg) {
 
                 final Table whereTable;
                 if (groupByColumnSources.length == 1) {
-                    whereTable = originalTable.where(new MatchFilter(groupByColumns[0], key));
+                    whereTable = originalTable.where(new MatchFilter(MatchType.Regular, groupByColumns[0], key));
                 } else {
                     final MatchFilter[] filters = new MatchFilter[groupByColumnSources.length];
                     for (int ii = 0; ii < groupByColumns.length; ++ii) {
-                        filters[ii] = new MatchFilter(groupByColumns[ii], key[ii]);
+                        filters[ii] = new MatchFilter(MatchType.Regular, groupByColumns[ii], key[ii]);
                     }
                     whereTable = originalTable.where(Filter.and(filters));
                 }

engine/table/src/test/java/io/deephaven/engine/table/impl/TestPartitioningColumns.java

@@ -12,6 +12,7 @@
 import io.deephaven.engine.table.*;
 import io.deephaven.engine.table.impl.dataindex.DataIndexUtils;
 import io.deephaven.engine.table.impl.indexer.DataIndexer;
+import io.deephaven.engine.table.impl.select.MatchFilter.MatchType;
 import io.deephaven.engine.table.iterators.ChunkedColumnIterator;
 import io.deephaven.engine.testutil.TstUtils;
 import io.deephaven.engine.testutil.junit4.EngineCleanup;
@@ -128,7 +129,8 @@ public void testEverything() {
         TstUtils.assertTableEquals(expected, result);
 
         final List<WhereFilter> filters = input.getDefinition().getColumnStream()
-                .map(cd -> new MatchFilter(cd.getName(), (Object) null)).collect(Collectors.toList());
+                .map(cd -> new MatchFilter(MatchType.Regular, cd.getName(), (Object) null))
+                .collect(Collectors.toList());
         TstUtils.assertTableEquals(expected.where(Filter.and(filters)), result.where(Filter.and(filters)));
 
         TstUtils.assertTableEquals(expected.selectDistinct(), result.selectDistinct());