Add inverse check to NumericValueRule for non-numeric values on NumberType fields

warehouse/query-core/src/main/java/datawave/query/jexl/visitors/FieldsWithNonNumericValuesVisitor.java

@@ -0,0 +1,91 @@
+package datawave.query.jexl.visitors;
+
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
+import org.apache.commons.jexl3.parser.ASTEQNode;
+import org.apache.commons.jexl3.parser.ASTGENode;
+import org.apache.commons.jexl3.parser.ASTGTNode;
+import org.apache.commons.jexl3.parser.ASTJexlScript;
+import org.apache.commons.jexl3.parser.ASTLENode;
+import org.apache.commons.jexl3.parser.ASTLTNode;
+import org.apache.commons.jexl3.parser.ASTNENode;
+import org.apache.commons.jexl3.parser.JexlNode;
+import org.apache.commons.lang3.math.NumberUtils;
+
+import datawave.query.jexl.JexlASTHelper;
+
+/**
+ * Visitor that collects fields whose comparison literal is a string that cannot be parsed as a number.
+ * <p>
+ * All simple comparison operators are inspected (equality, inequality, and the four range operators).
+ * Null and numeric literals are ignored. This is the inverse of {@link FieldsWithNumericValuesVisitor};
+ * see also {@link FieldsWithNumericRangeVisitor} for the range only numeric case.
+ */
+public class FieldsWithNonNumericValuesVisitor extends ShortCircuitBaseVisitor {
+
+    /**
+     * Fetch all fields that are compared against a non-numeric string literal.
+     * 
+     * @param query
+     *            The parsed JEXL query script
+     * @return ordered set of field names
+     */
+    @SuppressWarnings("unchecked")
+    public static Set<String> getFields(ASTJexlScript query) {
+        if (query == null) {
+            return Collections.emptySet();
+        } else {
+            FieldsWithNonNumericValuesVisitor visitor = new FieldsWithNonNumericValuesVisitor();
+            return (Set<String>) query.jjtAccept(visitor, new LinkedHashSet<String>());
+        }
+    }
+
+    @Override
+    public Object visit(ASTEQNode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTNENode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTLTNode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTGTNode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTLENode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTGENode node, Object data) {
+        checkSingleField(node, data);
+        return data;
+    }
+
+    @SuppressWarnings("unchecked")
+    private void checkSingleField(JexlNode node, Object data) {
+        String field = JexlASTHelper.getIdentifier(node);
+        if (field != null) {
+            Object literal = JexlASTHelper.getLiteralValueSafely(node);
+            if (literal instanceof String && !NumberUtils.isCreatable((String) literal)) {
+                ((Set<String>) data).add(field);
+            }
+        }
+    }
+}

warehouse/query-core/src/main/java/datawave/query/rules/NumericValueRule.java

@@ -10,6 +10,7 @@
 import com.google.common.collect.Multimap;
 
 import datawave.data.type.NumberType;
+import datawave.query.jexl.visitors.FieldsWithNonNumericValuesVisitor;
 import datawave.query.jexl.visitors.FieldsWithNumericRangeValuesVisitor;
 import datawave.query.util.TypeMetadata;
 
@@ -44,21 +45,20 @@ public QueryRuleResult validate(QueryValidationConfiguration ruleConfiguration)
         try {
             ASTJexlScript jexlScript = (ASTJexlScript) ruleConfig.getParsedQuery();
 
-            // Identify fields that participate in range comparisons (<, >, <=, >=), including those derived from Lucene ranges.
-            Set<String> fields = FieldsWithNumericRangeValuesVisitor.getFields(jexlScript);
-
-            // If no range fields were found, or no type metadata is available, nothing to validate.
             TypeMetadata typeMetadata = ruleConfig.getTypeMetadata();
             if (typeMetadata == null) {
                 throw new IllegalStateException("TypeMetadata should not be null.");
             }
-            if (!fields.isEmpty()) {
-                // A temporary cache to avoid unnecessary lookups via TypeMetadata if we see a field more than once.
-                Multimap<String,String> types = HashMultimap.create();
+            // A temporary cache to avoid unnecessary lookups via TypeMetadata if we see a field more than once.
+            Multimap<String,String> types = HashMultimap.create();
+
+            // Identify fields that participate in range comparisons (<, >, <=, >=), including those derived from Lucene ranges.
+            Set<String> rangeFields = FieldsWithNumericRangeValuesVisitor.getFields(jexlScript);
+            if (!rangeFields.isEmpty()) {
                 // Maintain insertion order.
                 Set<String> nonNumericFields = new LinkedHashSet<>();
                 // Find any fields that are not a number type.
-                for (String field : fields) {
+                for (String field : rangeFields) {
                     if (!types.containsKey(field)) {
                         types.putAll(field, typeMetadata.getNormalizerNamesForField(field));
                     }
@@ -73,6 +73,25 @@ public QueryRuleResult validate(QueryValidationConfiguration ruleConfiguration)
                     result.addMessage("Range values supplied for non-numeric field(s): " + String.join(", ", nonNumericFields));
                 }
             }
+
+            // Identify fields compared against a string literal that cannot be parsed as a number.
+            Set<String> nonNumericLiteralFields = FieldsWithNonNumericValuesVisitor.getFields(jexlScript);
+            if (!nonNumericLiteralFields.isEmpty()) {
+                // Maintain insertion order
+                Set<String> numericFields = new LinkedHashSet<>();
+                for (String field : nonNumericLiteralFields) {
+                    if (!types.containsKey(field)) {
+                        types.putAll(field, typeMetadata.getNormalizerNamesForField(field));
+                    }
+                    // If the field has NumberType as one of its normalizers, flag it.
+                    if (types.containsEntry(field, NUMBER_TYPE)) {
+                        numericFields.add(field);
+                    }
+                }
+                if (!numericFields.isEmpty()) {
+                    result.addMessage("Non-numeric values supplied for numeric field(s): " + String.join(", ", numericFields));
+                }
+            }
         } catch (Exception e) {
             log.error("Error occurred when validating against instance '" + getName() + "' of " + getClass(), e);
             result.setException(e);

warehouse/query-core/src/test/java/datawave/query/jexl/visitors/FieldsWithNonNumericValuesVisitorTest.java

@@ -0,0 +1,104 @@
+package datawave.query.jexl.visitors;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.commons.jexl3.parser.ASTJexlScript;
+import org.apache.commons.jexl3.parser.ParseException;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import datawave.query.jexl.JexlASTHelper;
+
+class FieldsWithNonNumericValuesVisitorTest {
+
+    private String query;
+    private final Set<String> expectedFields = new LinkedHashSet<>();
+
+    @AfterEach
+    void tearDown() {
+        query = null;
+        expectedFields.clear();
+    }
+
+    /**
+     * Test the various field operators with non-numeric string values.
+     * 
+     * @param operator
+     *            The operator
+     */
+    @ParameterizedTest
+    @ValueSource(strings = {"==", "!=", "<", ">", "<=", ">="})
+    void testOperatorsWithTextValue(String operator) throws ParseException {
+        givenQuery("FOO " + operator + " 'abc'");
+        expectFields("FOO");
+        assertResult();
+    }
+
+    /**
+     * Test the various field operators with numeric literal values.
+     *
+     * @param operator
+     *            The operator
+     */
+    @ParameterizedTest
+    @ValueSource(strings = {"==", "!=", "<", ">", "<=", ">="})
+    void testOperatorsWithNumericLiteral(String operator) throws ParseException {
+        givenQuery("FOO " + operator + " 1");
+
+        // Do not expect any fields.
+        assertResult();
+    }
+
+    /**
+     * Test string literals that are valid numbers.
+     */
+    @ParameterizedTest
+    @ValueSource(strings = {"'1'", "'2.0'", "'-5'", "'1e3'"})
+    void testFieldWithStringThatIsNumeric(String literal) throws ParseException {
+        givenQuery("FOO == " + literal);
+
+        // Do not expect any fields.
+        assertResult();
+    }
+
+    /**
+     * Test null literals are ignored.
+     */
+    @Test
+    void testNullLiteralIgnored() throws ParseException {
+        givenQuery("FOO == null");
+
+        // Do not expect any fields.
+        assertResult();
+    }
+
+    /**
+     * Test a mix of numeric and non-numeric literals across several fields.
+     */
+    @Test
+    void testMultipleFieldsWithMixedValues() throws ParseException {
+        givenQuery("FOO == 'abc' && BAR != '>60' || HAT > 3 || BAT < '5' || HEN == 'xyz'");
+        expectFields("FOO", "BAR", "HEN");
+        assertResult();
+    }
+
+    private void givenQuery(String query) {
+        this.query = query;
+    }
+
+    private void expectFields(String... fields) {
+        this.expectedFields.addAll(List.of(fields));
+    }
+
+    private void assertResult() throws ParseException {
+        ASTJexlScript script =JexlASTHelper.parseJexlQuery(query);
+        Set<String> actual = FieldsWithNonNumericValuesVisitor.getFields(script);
+        assertEquals(expectedFields, actual);
+    }
+}

warehouse/query-core/src/test/java/datawave/query/rules/NumericValueRuleTest.java

@@ -98,6 +98,7 @@ void testLuceneRangesConvertedAndValidated() throws Exception {
         TypeMetadata typeMetadata = EasyMock.mock(TypeMetadata.class);
         EasyMock.expect(typeMetadata.getNormalizerNamesForField("A")).andReturn(NUMBER_TYPE);
         EasyMock.expect(typeMetadata.getNormalizerNamesForField("B")).andReturn(LC_NO_DIACRITICS_TYPE);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("C")).andReturn(LC_NO_DIACRITICS_TYPE);
         EasyMock.expect(typeMetadata.getNormalizerNamesForField("D")).andReturn(LC_NO_DIACRITICS_TYPE);
         EasyMock.replay(typeMetadata);
         givenTypeMetadata(typeMetadata);
@@ -139,6 +140,38 @@ void testNegatedRangeStillValidated() throws Exception {
         assertResult();
     }
 
+    @Test
+    void testNonNumericValuesForNumericFields() throws Exception {
+        givenQuery("FOO == '>60' && BAR != 'abc' && BAZ == 'xyz'");
+
+        TypeMetadata typeMetadata = EasyMock.mock(TypeMetadata.class);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("FOO")).andReturn(NUMBER_TYPE);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("BAR")).andReturn(NUMBER_TYPE);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("BAZ")).andReturn(LC_NO_DIACRITICS_TYPE);
+        EasyMock.replay(typeMetadata);
+        givenTypeMetadata(typeMetadata);
+
+        expectMessage("Non-numeric values supplied for numeric field(s): FOO, BAR");
+
+        assertResult();
+    }
+
+    @Test
+    void testBothDirectionsReportedTogether() throws Exception {
+        givenQuery("FOO == 'abc' && BAR > 5");
+
+        TypeMetadata typeMetadata = EasyMock.mock(TypeMetadata.class);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("FOO")).andReturn(NUMBER_TYPE);
+        EasyMock.expect(typeMetadata.getNormalizerNamesForField("BAR")).andReturn(LC_NO_DIACRITICS_TYPE);
+        EasyMock.replay(typeMetadata);
+        givenTypeMetadata(typeMetadata);
+
+        expectMessage("Range values supplied for non-numeric field(s): BAR");
+        expectMessage("Non-numeric values supplied for numeric field(s): FOO");
+
+        assertResult();
+    }
+
     @Override
     protected Object parseQuery() throws Exception {
         return parseQueryToJexl();