From 39dd38f0cfb55126c41f6f193f8e57495c18438b Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Fri, 17 Nov 2017 13:37:30 +0100 Subject: 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. --- .../java/com/yahoo/document/select/ResultList.java | 4 +++ .../yahoo/document/select/rule/ArithmeticNode.java | 2 +- .../yahoo/document/select/rule/AttributeNode.java | 4 ++- .../yahoo/document/select/rule/ComparisonNode.java | 21 ++++++++++-- .../document/select/DocumentSelectorTestCase.java | 39 +++++++++++++++++++--- 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))); -- cgit v1.2.3