Fix handling of missing fields in Java document selection matching

Now matches C++ implementation by returning a null value when attempting
to match a missing field rather than returning an empty result list.
Altered handling of null comparisons; they should now have the expected
semantics.

5 files changed, 61 insertions, 9 deletions

diff --git a/document/src/main/java/com/yahoo/document/select/ResultList.java b/document/src/main/java/com/yahoo/document/select/ResultList.java
index 41f0fb5edef..8d73c475981 100644
--- a/document/src/main/java/com/yahoo/document/select/ResultList.java
+++ b/document/src/main/java/com/yahoo/document/select/ResultList.java
@@ -60,6 +60,10 @@ public class ResultList {
         return results;
     }
 
+    public static ResultList fromBoolean(boolean result) {
+        return new ResultList(result ? Result.TRUE : Result.FALSE);
+    }
+
     public Result toResult() {
         if (results.isEmpty()) {
             return Result.FALSE;
diff --git a/document/src/main/java/com/yahoo/document/select/rule/ArithmeticNode.java b/document/src/main/java/com/yahoo/document/select/rule/ArithmeticNode.java
index 34bcf914d17..a54f5cada97 100644
--- a/document/src/main/java/com/yahoo/document/select/rule/ArithmeticNode.java
+++ b/document/src/main/java/com/yahoo/document/select/rule/ArithmeticNode.java
@@ -50,7 +50,7 @@ public class ArithmeticNode implements ExpressionNode {
             Object val = item.node.evaluate(context);
 
             if (val == null) {
-                throw new IllegalStateException("Null value found!");
+                throw new IllegalArgumentException("Can not perform arithmetic on null value (referencing missing field?)");
             }
 
             if (val instanceof AttributeNode.VariableValueList) {
diff --git a/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java b/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java
index b8281b9ed0a..bbd244dd1dc 100644
--- a/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java
+++ b/document/src/main/java/com/yahoo/document/select/rule/AttributeNode.java
@@ -127,12 +127,14 @@ public class AttributeNode implements ExpressionNode {
             FieldPath fieldPath = doc.getDataType().buildFieldPath(fieldPth);
             IteratorHandler handler = new IteratorHandler();
             doc.iterateNested(fieldPath, 0, handler);
+            if (handler.values.isEmpty()) {
+                return null;
+            }
             return handler.values;
         } else if (value instanceof DocumentUpdate) {
             return Result.INVALID;
         }
         return Result.FALSE;
-        //throw new IllegalStateException("Attributes are only available for document types for value '" + value + "'. Looking for " + fieldPth);
     }
 
     private static Object evaluateFunction(String function, Object value) {
diff --git a/document/src/main/java/com/yahoo/document/select/rule/ComparisonNode.java b/document/src/main/java/com/yahoo/document/select/rule/ComparisonNode.java
index a13641adadf..372b61bb493 100644
--- a/document/src/main/java/com/yahoo/document/select/rule/ComparisonNode.java
+++ b/document/src/main/java/com/yahoo/document/select/rule/ComparisonNode.java
@@ -215,8 +215,8 @@ public class ComparisonNode implements ExpressionNode {
     public Object evaluate(Context context) {
         Object oLeft = lhs.evaluate(context);
         Object oRight = rhs.evaluate(context);
-        if (oLeft == null && oRight == null) {
-            return new ResultList(Result.TRUE);
+        if (oLeft == null || oRight == null) {
+            return evaluateWithAtLeastOneNullSide(oLeft, oRight);
         }
         if (oLeft == Result.INVALID || oRight == Result.INVALID) {
             return new ResultList(Result.INVALID);
@@ -237,6 +237,23 @@ public class ComparisonNode implements ExpressionNode {
         return new ResultList(evaluateBool(oLeft, oRight));
     }
 
+    /**
+     * Evaluates a binary comparison where one or both operands are null.
+     * Boolean outcomes are only defined for (in)equality relations, all others
+     * return Result.INVALID.
+     *
+     * Precondition: lhs AND/OR rhs is null.
+     */
+    private ResultList evaluateWithAtLeastOneNullSide(Object lhs, Object rhs) {
+        if (operator.equals("==") || operator.equals("=")) { // Glob (=) operator falls back to equality for non-strings
+            return ResultList.fromBoolean(lhs == rhs);
+        } else if (operator.equals("!=")) {
+            return ResultList.fromBoolean(lhs != rhs);
+        } else {
+            return new ResultList(Result.INVALID);
+        }
+    }
+
     public ResultList evaluateListsTrue(AttributeNode.VariableValueList lhs, AttributeNode.VariableValueList rhs) {
         if (lhs.size() != rhs.size()) {
             return new ResultList(Result.FALSE);
diff --git a/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java b/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java
index 7b6130185b4..508bf7f0b18 100644
--- a/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java
+++ b/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java
@@ -263,7 +263,7 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.FALSE, evaluate("test.content = 1 and true", put)); // BROKEN
         assertEquals(Result.INVALID, evaluate("test.content = 1 and true", upd));
 
-        assertEquals(Result.FALSE, evaluate("test.content = 1 or true", put)); // BROKEN
+        assertEquals(Result.TRUE, evaluate("test.content = 1 or true", put));
         assertEquals(Result.TRUE, evaluate("test.content = 1 or true", upd));
 
         assertEquals(Result.FALSE, evaluate("test.content = 1 and false",  put));
@@ -275,7 +275,7 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.FALSE, evaluate("true and test.content = 1",  put)); // BROKEN
         assertEquals(Result.INVALID, evaluate("true and test.content = 1",  upd));
 
-        assertEquals(Result.FALSE, evaluate("true or test.content = 1", put)); // BROKEN
+        assertEquals(Result.TRUE, evaluate("true or test.content = 1", put));
         assertEquals(Result.TRUE, evaluate("true or test.content = 1", upd));
 
         assertEquals(Result.FALSE, evaluate("false and test.content = 1",  put));
@@ -420,8 +420,10 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.TRUE, evaluate("30 != \"foo\"", documents.get(0)));
         assertEquals(Result.INVALID, evaluate("14.2 <= \"foo\"", documents.get(0)));
         assertEquals(Result.TRUE, evaluate("null == null", documents.get(0)));
-        assertEquals(Result.TRUE, evaluate("null = null", documents.get(0)));
+        assertEquals(Result.TRUE, evaluate("null = null", documents.get(0))); // Glob operator falls back to == comparison
+        assertEquals(Result.FALSE, evaluate("null != null", documents.get(0)));
         assertEquals(Result.FALSE, evaluate("\"bar\" == null", documents.get(0)));
+        assertEquals(Result.FALSE, evaluate("null == \"bar\"", documents.get(0)));
         assertEquals(Result.FALSE, evaluate("14.3 == null", documents.get(0)));
         assertEquals(Result.FALSE, evaluate("null = 0", documents.get(0)));
 
@@ -441,6 +443,20 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.FALSE, evaluate("test.hstring == test.content", documents.get(0)));
         assertEquals(Result.TRUE, evaluate("test.hstring == test.content", documents.get(2)));
         assertEquals(Result.TRUE, evaluate("test.hint + 1 > 13", documents.get(1)));
+        // Case where field is not present (i.e. null) is defined for (in)equality comparisons, but
+        // not for other relations.
+        assertEquals(Result.TRUE, evaluate("test.hint != 1234", documents.get(7)));
+        assertEquals(Result.FALSE, evaluate("test.hint == 1234", documents.get(7)));
+        assertEquals(Result.INVALID, evaluate("test.hint < 1234", documents.get(7)));
+        // Propagation of Invalid through logical operators should match C++ implementation
+        assertEquals(Result.FALSE, evaluate("test.hint < 1234 and false", documents.get(7)));
+        assertEquals(Result.INVALID, evaluate("test.hint < 1234 and true", documents.get(7)));
+        assertEquals(Result.TRUE, evaluate("test.hint < 1234 or true", documents.get(7)));
+        assertEquals(Result.INVALID, evaluate("test.hint < 1234 or false", documents.get(7)));
+        // Must be possible to predicate a sub-expression on the presence of a field without
+        // propagating up an Invalid value from the comparison.
+        assertEquals(Result.FALSE, evaluate("test.hint and test.hint < 1234", documents.get(7)));
+        assertEquals(Result.FALSE, evaluate("test.hint != null and test.hint < 1234", documents.get(7)));
 
         // Document types.
         assertEquals(Result.TRUE, evaluate("test", documents.get(0)));
@@ -457,6 +473,19 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.TRUE, evaluate("not test.hint", documents.get(7)));
         assertEquals(Result.TRUE, evaluate("not test.hstring", documents.get(7)));
 
+        assertEquals(Result.TRUE, evaluate("test.hint != null", documents.get(0)));
+        assertEquals(Result.TRUE, evaluate("null != test.hint", documents.get(0)));
+        assertEquals(Result.FALSE, evaluate("test.hint == null", documents.get(0)));
+        assertEquals(Result.FALSE, evaluate("null == test.hint", documents.get(0)));
+        assertEquals(Result.TRUE, evaluate("null == test.hint", documents.get(7)));
+        assertEquals(Result.TRUE, evaluate("test.hint == null", documents.get(7)));
+        assertEquals(Result.FALSE, evaluate("test.hint != null", documents.get(7)));
+        assertEquals(Result.FALSE, evaluate("null != test.hint", documents.get(7)));
+
+        assertEquals(Result.TRUE, evaluate("test.hint or true", documents.get(7)));
+        assertEquals(Result.TRUE, evaluate("not test.hint and true", documents.get(7)));
+        assertEquals(Result.FALSE, evaluate("not test.hint and false", documents.get(7)));
+
         // Id values.
         assertEquals(Result.TRUE, evaluate("id == \"doc:myspace:anything\"", documents.get(0)));
         assertEquals(Result.TRUE, evaluate(" iD==  \"doc:myspace:anything\"  ", documents.get(0)));
@@ -548,7 +577,7 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         assertEquals(Result.TRUE, evaluate("test.mystruct == test.mystruct", documents.get(1)));
         assertEquals(Result.FALSE, evaluate("test.mystruct != test.mystruct", documents.get(0)));
         assertEquals(Result.FALSE, evaluate("test.mystruct != test.mystruct", documents.get(1)));
-        //assertEquals(Result.INVALID, evaluate("test.mystruct < test.mystruct", documents.get(0)));
+        assertEquals(Result.INVALID, evaluate("test.mystruct < test.mystruct", documents.get(0)));
         //assertEquals(Result.FALSE, evaluate("test.mystruct < test.mystruct", documents.get(1)));
         //assertEquals(Result.INVALID, evaluate("test.mystruct < 5", documents.get(1)));
         //assertEquals(Result.INVALID, evaluate("test.mystruct == \"foo\"", documents.get(1)));
@@ -584,7 +613,7 @@ public class DocumentSelectorTestCase extends junit.framework.TestCase {
         // Globbing/regexp of struct fields
         assertEquals(Result.FALSE, evaluate("test.mystruct.value = \"struc?val\"", documents.get(0)));
         assertEquals(Result.TRUE, evaluate("test.mystruct.value = \"struc?val\"", documents.get(1)));
-        assertEquals(Result.FALSE, evaluate("test.mystruct.value =~ \"struct.*\"", documents.get(0)));
+        assertEquals(Result.INVALID, evaluate("test.mystruct.value =~ \"struct.*\"", documents.get(0))); // Invalid since lhs is null
         assertEquals(Result.TRUE, evaluate("test.mystruct.value =~ \"struct.*\"", documents.get(1)));
 
         assertEquals(Result.FALSE, evaluate("test.intarray < 5", documents.get(0)));