diff options
26 files changed, 213 insertions, 169 deletions
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 cf7e3851d55..d6166f8fcbe 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 @@ -282,6 +282,8 @@ public class ComparisonNode implements ExpressionNode { return getAsNumber(((NumericFieldValue)value).getNumber()); } else if (value instanceof BoolFieldValue) { return ((BoolFieldValue)value).getBoolean() ? 1 : 0; + } else if (value instanceof Boolean) { + return (Boolean)value ? 1 : 0; } else { return Double.NaN; //new IllegalStateException("Term '" + value + "' (" + value.getClass() + ") does not evaluate to a number."); } 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 bf8cf07a097..38fdadb18e4 100644 --- a/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java +++ b/document/src/test/java/com/yahoo/document/select/DocumentSelectorTestCase.java @@ -507,6 +507,16 @@ public class DocumentSelectorTestCase { assertEquals(Result.FALSE, evaluate("14.3 == null", documents.get(0))); assertEquals(Result.FALSE, evaluate("null = 0", documents.get(0))); + // Boolean literals in comparisons + assertEquals(Result.TRUE, evaluate("true = true", documents.get(0))); + assertEquals(Result.TRUE, evaluate("true == true", documents.get(0))); + assertEquals(Result.FALSE, evaluate("true == false", documents.get(0))); + assertEquals(Result.TRUE, evaluate("false == false", documents.get(0))); + assertEquals(Result.TRUE, evaluate("true == 1", documents.get(0))); + assertEquals(Result.FALSE, evaluate("true == 0", documents.get(0))); + assertEquals(Result.FALSE, evaluate("false == 1", documents.get(0))); + assertEquals(Result.TRUE, evaluate("false == 0", documents.get(0))); + // Field values assertEquals(Result.TRUE, evaluate("test.hint = 24", documents.get(0))); assertEquals(Result.FALSE, evaluate("test.hint = 24", documents.get(1))); @@ -748,12 +758,19 @@ public class DocumentSelectorTestCase { @Test public void boolean_fields_can_be_used_for_equality_comparisons() throws ParseException { var documents = createDocs(); - assertEquals(Result.TRUE, evaluate("test.truth == 1", documents.get(8))); // has explicit field set to true - assertEquals(Result.FALSE, evaluate("test.truth == 1", documents.get(9))); // has explicit field set to false + // Doc 8 has bool field set explicitly to true, doc 9 has field explicitly set to false + assertEquals(Result.TRUE, evaluate("test.truth == 1", documents.get(8))); + assertEquals(Result.TRUE, evaluate("test.truth == true", documents.get(8))); + assertEquals(Result.FALSE, evaluate("test.truth == 1", documents.get(9))); + assertEquals(Result.FALSE, evaluate("test.truth == true", documents.get(9))); assertEquals(Result.TRUE, evaluate("test.truth == 0", documents.get(9))); + assertEquals(Result.TRUE, evaluate("test.truth == false", documents.get(9))); // FIXME very un-intuitive behavior when nulls are implicitly returned: - assertEquals(Result.FALSE, evaluate("test.truth == 1", documents.get(0))); // Does not have field set in document - assertEquals(Result.FALSE, evaluate("test.truth == 0", documents.get(0))); // Does not have field set in document + // Doc 1 does not have the bool field set, but the implicit null value is neither true nor false + assertEquals(Result.FALSE, evaluate("test.truth == 1", documents.get(0))); + assertEquals(Result.FALSE, evaluate("test.truth == true", documents.get(0))); + assertEquals(Result.FALSE, evaluate("test.truth == 0", documents.get(0))); + assertEquals(Result.FALSE, evaluate("test.truth == false", documents.get(0))); } @Test diff --git a/document/src/tests/documentselectparsertest.cpp b/document/src/tests/documentselectparsertest.cpp index 6644fec2da0..1606cbc634d 100644 --- a/document/src/tests/documentselectparsertest.cpp +++ b/document/src/tests/documentselectparsertest.cpp @@ -637,6 +637,16 @@ TEST_F(DocumentSelectParserTest, operators_1) PARSE("14.3 == null", *_doc[0], False); PARSE("null = 0", *_doc[0], False); + // Boolean literals in comparisons + PARSE("true = true", *_doc[0], True); + PARSE("true == true", *_doc[0], True); + PARSE("true == false", *_doc[0], False); + PARSE("false == false", *_doc[0], True); + PARSE("true == 1", *_doc[0], True); + PARSE("true == 0", *_doc[0], False); + PARSE("false == 1", *_doc[0], False); + PARSE("false == 0", *_doc[0], True); + // Field values PARSE("testdoctype1.headerval = 24", *_doc[0], True); PARSE("testdoctype1.headerval = 24", *_doc[1], False); @@ -925,12 +935,19 @@ TEST_F(DocumentSelectParserTest, operators_9) TEST_F(DocumentSelectParserTest, can_use_boolean_fields_in_expressions) { createDocs(); - PARSE("testdoctype1.boolfield == 1", *_doc[11], True); // has explicit field set to true - PARSE("testdoctype1.boolfield == 1", *_doc[12], False); // has explicit field set to false + // Doc 11 has bool field set explicitly to true, doc 12 has field explicitly set to false + PARSE("testdoctype1.boolfield == 1", *_doc[11], True); + PARSE("testdoctype1.boolfield == true", *_doc[11], True); + PARSE("testdoctype1.boolfield == 1", *_doc[12], False); + PARSE("testdoctype1.boolfield == true", *_doc[12], False); PARSE("testdoctype1.boolfield == 0", *_doc[12], True); + PARSE("testdoctype1.boolfield == false", *_doc[12], True); // FIXME very un-intuitive behavior when nulls are implicitly returned: - PARSE("testdoctype1.boolfield == 1", *_doc[1], False); // Does not have field set in document - PARSE("testdoctype1.boolfield == 0", *_doc[1], False); // Does not have field set in document + // Doc 1 does not have the bool field set, but the implicit null value is neither true nor false + PARSE("testdoctype1.boolfield == 1", *_doc[1], False); + PARSE("testdoctype1.boolfield == true", *_doc[1], False); + PARSE("testdoctype1.boolfield == 0", *_doc[1], False); + PARSE("testdoctype1.boolfield == false", *_doc[1], False); } namespace { @@ -989,6 +1006,7 @@ namespace { void visitFloatValueNode(const select::FloatValueNode &) override {} void visitVariableValueNode(const select::VariableValueNode &) override {} void visitIntegerValueNode(const select::IntegerValueNode &) override {} + void visitBoolValueNode(const select::BoolValueNode &) override {} void visitCurrentTimeValueNode(const select::CurrentTimeValueNode &) override {} void visitStringValueNode(const select::StringValueNode &) override {} void visitNullValueNode(const select::NullValueNode &) override {} @@ -1392,6 +1410,9 @@ public: void visitIntegerValueNode(const select::IntegerValueNode& node) override { data << node.getValue(); } + void visitBoolValueNode(const select::BoolValueNode& node) override { + data << node.bool_value_str(); + } void visitCurrentTimeValueNode(const select::CurrentTimeValueNode&) override {} void visitStringValueNode(const select::StringValueNode& str) override { data << '"' << str.getValue() << '"'; @@ -1491,6 +1512,17 @@ TEST_F(DocumentSelectParserTest, test_ambiguous_field_spec_expression_is_handled parse_to_tree("(testdoctype1.foo) AND (testdoctype1.bar)")); } +TEST_F(DocumentSelectParserTest, test_ambiguous_bool_expression_is_handled_correctly) +{ + createDocs(); + using namespace std::string_literals; + // Bools both as high level Nodes and low level ValueNodes + EXPECT_EQ("(OR (AND true false) (== (FIELD testdoctype1 myfield) true))"s, + parse_to_tree("true and false or testdoctype1.myfield == true")); + EXPECT_EQ("(!= true false)"s, parse_to_tree("true != false")); + EXPECT_EQ("(!= true false)"s, parse_to_tree("(true) != (false)")); +} + TEST_F(DocumentSelectParserTest, special_tokens_are_allowed_as_freestanding_identifier_names) { createDocs(); EXPECT_EQ("(NOT (DOCTYPE user))", parse_to_tree("not user")); diff --git a/document/src/vespa/document/bucket/bucketselector.cpp b/document/src/vespa/document/bucket/bucketselector.cpp index 34f613bef84..b9dc6a9cfa9 100644 --- a/document/src/vespa/document/bucket/bucketselector.cpp +++ b/document/src/vespa/document/bucket/bucketselector.cpp @@ -152,6 +152,7 @@ using namespace document::select; void visitFloatValueNode(const FloatValueNode &) override {} void visitVariableValueNode(const VariableValueNode &) override {} void visitIntegerValueNode(const IntegerValueNode &) override {} + void visitBoolValueNode(const BoolValueNode &) override {} void visitCurrentTimeValueNode(const CurrentTimeValueNode &) override {} void visitStringValueNode(const StringValueNode &) override {} void visitNullValueNode(const NullValueNode &) override {} diff --git a/document/src/vespa/document/select/cloningvisitor.cpp b/document/src/vespa/document/select/cloningvisitor.cpp index 55e8c1effc1..81cb31145ec 100644 --- a/document/src/vespa/document/select/cloningvisitor.cpp +++ b/document/src/vespa/document/select/cloningvisitor.cpp @@ -10,30 +10,6 @@ namespace document::select { -const int CloningVisitor::OrPriority; -const int CloningVisitor::AndPriority; -const int CloningVisitor::NotPriority; -const int CloningVisitor::ComparePriority; -const int CloningVisitor::AddPriority; -const int CloningVisitor::SubPriority; -const int CloningVisitor::MulPriority; -const int CloningVisitor::DivPriority; -const int CloningVisitor::ModPriority; -const int CloningVisitor::DocumentTypePriority; -const int CloningVisitor::FieldValuePriority; -const int CloningVisitor::InvalidConstPriority; -const int CloningVisitor::InvalidValPriority; -const int CloningVisitor::ConstPriority; -const int CloningVisitor::FuncPriority; -const int CloningVisitor::VariablePriority; -const int CloningVisitor::FloatPriority; -const int CloningVisitor::IntegerPriority; -const int CloningVisitor::CurrentTimePriority; -const int CloningVisitor::StringPriority; -const int CloningVisitor::NullValPriority; -const int CloningVisitor::IdPriority; -const int CloningVisitor::SearchColPriority; - CloningVisitor::CloningVisitor() : _node(), _valueNode(), @@ -63,7 +39,7 @@ CloningVisitor::visitAndBranch(const And &expr) setNodeParentheses(priority); std::unique_ptr<Node> rhs(std::move(_node)); _priority = priority; - _node.reset(new And(std::move(lhs), std::move(rhs), "and")); + _node = std::make_unique<And>(std::move(lhs), std::move(rhs), "and"); }; @@ -83,7 +59,7 @@ CloningVisitor::visitOrBranch(const Or &expr) setNodeParentheses(priority); std::unique_ptr<Node> rhs(std::move(_node)); _priority = priority; - _node.reset(new Or(std::move(lhs), std::move(rhs), "or")); + _node = std::make_unique<Or>(std::move(lhs), std::move(rhs), "or"); }; @@ -96,7 +72,7 @@ CloningVisitor::visitNotBranch(const Not &expr) _resultSet.calcNot(); std::unique_ptr<Node> child(std::move(_node)); _priority = priority; - _node.reset(new Not(std::move(child), "not")); + _node = std::make_unique<Not>(std::move(child), "not"); }; @@ -116,10 +92,7 @@ CloningVisitor::visitComparison(const Compare &expr) const Operator &op(expr.getOperator()); _priority = priority; _resultSet.fill(); // should be less if const - _node.reset(new Compare(std::move(lhs), - op, - std::move(rhs), - expr.getBucketIdFactory())); + _node = std::make_unique<Compare>(std::move(lhs), op, std::move(rhs), expr.getBucketIdFactory()); }; @@ -149,7 +122,7 @@ CloningVisitor::visitFunctionValueNode(const FunctionValueNode &expr) setValueNodeParentheses(priority); std::unique_ptr<ValueNode> child(std::move(_valueNode)); _priority = priority; - _valueNode.reset(new FunctionValueNode(expr.getFunctionName(), std::move(child))); + _valueNode = std::make_unique<FunctionValueNode>(expr.getFunctionName(), std::move(child)); }; @@ -160,7 +133,7 @@ CloningVisitor::visitConstant(const Constant &expr) _priority = ConstPriority; bool val = expr.getConstantValue(); _resultSet.add(val ? Result::True : Result::False); - _node.reset(new Constant(val)); + _node = std::make_unique<Constant>(val); } @@ -171,7 +144,7 @@ CloningVisitor::visitInvalidConstant(const InvalidConstant &expr) _constVal = true; _priority = InvalidConstPriority; _resultSet.add(Result::Invalid); - _node.reset(new InvalidConstant("invalid")); + _node = std::make_unique<InvalidConstant>("invalid"); } @@ -218,7 +191,7 @@ CloningVisitor::visitFloatValueNode(const FloatValueNode &expr) void CloningVisitor::visitVariableValueNode(const VariableValueNode &expr) { - _valueNode.reset(new VariableValueNode(expr.getVariableName())); + _valueNode = std::make_unique<VariableValueNode>(expr.getVariableName()); _priority = VariablePriority; } @@ -233,6 +206,15 @@ CloningVisitor::visitIntegerValueNode(const IntegerValueNode &expr) void +CloningVisitor::visitBoolValueNode(const BoolValueNode &expr) +{ + _constVal = true; + _valueNode = expr.clone(); + _priority = BoolPriority; +} + + +void CloningVisitor::visitCurrentTimeValueNode(const CurrentTimeValueNode &expr) { _constVal = false; diff --git a/document/src/vespa/document/select/cloningvisitor.h b/document/src/vespa/document/select/cloningvisitor.h index a49f44190ef..cce171f81cb 100644 --- a/document/src/vespa/document/select/cloningvisitor.h +++ b/document/src/vespa/document/select/cloningvisitor.h @@ -20,33 +20,33 @@ protected: uint32_t _fieldNodes; ResultSet _resultSet; - static const int OrPriority = 100; - static const int AndPriority = 200; - static const int NotPriority = 300; - static const int ComparePriority = 400; - static const int AddPriority = 500; - static const int SubPriority = 500; - static const int MulPriority = 600; - static const int DivPriority = 600; - static const int ModPriority = 700; - static const int DocumentTypePriority = 1000; - static const int FieldValuePriority = 1000; - static const int InvalidConstPriority = 1000; - static const int InvalidValPriority = 1000; - static const int ConstPriority = 1000; - static const int FuncPriority = 1000; - static const int VariablePriority = 1000; - static const int FloatPriority = 1000; - static const int IntegerPriority = 1000; - static const int CurrentTimePriority = 1000; - static const int StringPriority = 1000; - static const int NullValPriority = 1000; - static const int IdPriority = 1000; - static const int SearchColPriority = 1000; + static constexpr int OrPriority = 100; + static constexpr int AndPriority = 200; + static constexpr int NotPriority = 300; + static constexpr int ComparePriority = 400; + static constexpr int AddPriority = 500; + static constexpr int SubPriority = 500; + static constexpr int MulPriority = 600; + static constexpr int DivPriority = 600; + static constexpr int ModPriority = 700; + static constexpr int DocumentTypePriority = 1000; + static constexpr int FieldValuePriority = 1000; + static constexpr int InvalidConstPriority = 1000; + static constexpr int InvalidValPriority = 1000; + static constexpr int ConstPriority = 1000; + static constexpr int FuncPriority = 1000; + static constexpr int VariablePriority = 1000; + static constexpr int FloatPriority = 1000; + static constexpr int IntegerPriority = 1000; + static constexpr int BoolPriority = 1000; + static constexpr int CurrentTimePriority = 1000; + static constexpr int StringPriority = 1000; + static constexpr int NullValPriority = 1000; + static constexpr int IdPriority = 1000; public: CloningVisitor(); - ~CloningVisitor(); + ~CloningVisitor() override; void visitAndBranch(const And &expr) override; void visitOrBranch(const Or &expr) override; @@ -62,6 +62,7 @@ public: void visitFloatValueNode(const FloatValueNode &expr) override; void visitVariableValueNode(const VariableValueNode &expr) override; void visitIntegerValueNode(const IntegerValueNode &expr) override; + void visitBoolValueNode(const BoolValueNode &expr) override; void visitCurrentTimeValueNode(const CurrentTimeValueNode &expr) override; void visitStringValueNode(const StringValueNode &expr) override; void visitNullValueNode(const NullValueNode &expr) override; @@ -77,7 +78,7 @@ public: int rhsPriority, bool rhsConstVal); void swap(CloningVisitor &rhs); - void revisit(void); + void revisit(); }; } diff --git a/document/src/vespa/document/select/gid_filter.cpp b/document/src/vespa/document/select/gid_filter.cpp index 3a3e215db68..a1954eb6bc2 100644 --- a/document/src/vespa/document/select/gid_filter.cpp +++ b/document/src/vespa/document/select/gid_filter.cpp @@ -26,6 +26,7 @@ struct NoOpVisitor : Visitor { void visitFloatValueNode(const FloatValueNode&) override {} void visitVariableValueNode(const VariableValueNode&) override {} void visitIntegerValueNode(const IntegerValueNode&) override {} + void visitBoolValueNode(const BoolValueNode&) override {} void visitCurrentTimeValueNode(const CurrentTimeValueNode&) override {} void visitStringValueNode(const StringValueNode&) override {} void visitNullValueNode(const NullValueNode&) override {} diff --git a/document/src/vespa/document/select/grammar/parser.yy b/document/src/vespa/document/select/grammar/parser.yy index 8d64b2382b5..409f8e3b29e 100644 --- a/document/src/vespa/document/select/grammar/parser.yy +++ b/document/src/vespa/document/select/grammar/parser.yy @@ -75,7 +75,7 @@ %token <string_val> ID USER GROUP SCHEME NAMESPACE SPECIFIC BUCKET GID TYPE %type <string_val> ident mangled_ident -%type <abstract_node> bool_ +%type <value_node> bool_ /* TODO 'leaf' is a bad name for something that isn't a leaf... */ %type <abstract_node> expression comparison logical_expr leaf doc_type %type <string_val> id_arg @@ -190,8 +190,8 @@ null_ ; bool_ - : TRUE { $$ = new Constant(true); } - | FALSE { $$ = new Constant(false); } + : TRUE { $$ = new BoolValueNode(true); } + | FALSE { $$ = new BoolValueNode(false); } ; number @@ -287,6 +287,7 @@ field_spec value : null_ { $$ = $1; } + | bool_ { $$ = $1; } | string { $$ = $1; } | id_spec { $$ = $1; } | variable { $$ = $1; } @@ -328,10 +329,9 @@ comparison ; leaf - : bool_ { $$ = $1; } - | comparison { $$ = $1; } + : comparison { $$ = $1; } | doc_type { $$ = $1; } - | arith_expr { /* Actually field_spec, see comment below..! */ + | arith_expr { /* Actually bool_ or field_spec, see comment below..! */ // Grammar-wise, we _do not_ accept arbitrary arith_exprs at this level. But the // selection grammar as it stands is otherwise ambiguous with LR(1) parsing. // More specifically, if we used field_spec instead of arith_expr here, the parser @@ -343,15 +343,21 @@ leaf // conflict goes away. We can then do a sneaky "run-time" type check to ensure we only // get the expected node from the rule. // It's not pretty, but it avoids an undefined grammar (which is much less pretty!). + // The same goes for boolean constants, which may be used both as higher-level (non-value) + // nodes or as value nodes to compare against. auto node = steal<ValueNode>($1); - if (dynamic_cast<FieldValueNode*>(node.get()) == nullptr) { - throw syntax_error(@$, "expected field spec, doctype, bool or comparison"); + if (auto* as_bool = dynamic_cast<BoolValueNode*>(node.get())) { + $$ = new Constant(as_bool->bool_value()); // Replace single bool node subtree with constant. + } else { + if (dynamic_cast<FieldValueNode*>(node.get()) == nullptr) { + throw syntax_error(@$, "expected field spec, doctype, bool or comparison"); + } + // Implicit rewrite to non-null comparison node + $$ = new Compare(std::move(node), + FunctionOperator::NE, + std::make_unique<NullValueNode>(), + bucket_id_factory); } - // Implicit rewrite to non-null comparison node - $$ = new Compare(std::move(node), - FunctionOperator::NE, - std::make_unique<NullValueNode>(), - bucket_id_factory); } ; diff --git a/document/src/vespa/document/select/traversingvisitor.cpp b/document/src/vespa/document/select/traversingvisitor.cpp index a0b7441903e..f7e76e6fe32 100644 --- a/document/src/vespa/document/select/traversingvisitor.cpp +++ b/document/src/vespa/document/select/traversingvisitor.cpp @@ -97,6 +97,12 @@ TraversingVisitor::visitIntegerValueNode(const IntegerValueNode &) void +TraversingVisitor::visitBoolValueNode(const BoolValueNode &) +{ +} + + +void TraversingVisitor::visitCurrentTimeValueNode(const CurrentTimeValueNode &) { } diff --git a/document/src/vespa/document/select/traversingvisitor.h b/document/src/vespa/document/select/traversingvisitor.h index be240c42537..2a7531c06f0 100644 --- a/document/src/vespa/document/select/traversingvisitor.h +++ b/document/src/vespa/document/select/traversingvisitor.h @@ -24,6 +24,7 @@ public: void visitFloatValueNode(const FloatValueNode &) override; void visitVariableValueNode(const VariableValueNode &) override; void visitIntegerValueNode(const IntegerValueNode &) override; + void visitBoolValueNode(const BoolValueNode &) override; void visitCurrentTimeValueNode(const CurrentTimeValueNode &) override; void visitStringValueNode(const StringValueNode &) override; void visitNullValueNode(const NullValueNode &) override; diff --git a/document/src/vespa/document/select/valuenode.h b/document/src/vespa/document/select/valuenode.h index 71efa73b1d8..fb86128aa2c 100644 --- a/document/src/vespa/document/select/valuenode.h +++ b/document/src/vespa/document/select/valuenode.h @@ -57,12 +57,11 @@ protected: } } - ValueNode::UP wrapParens(ValueNode* node) const { - ValueNode::UP ret(node); + std::unique_ptr<ValueNode> wrapParens(std::unique_ptr<ValueNode> node) const { if (_parentheses) { - ret->setParentheses(); + node->setParentheses(); } - return ret; + return node; } std::unique_ptr<Value> defaultTrace(std::unique_ptr<Value> val, std::ostream& out) const; diff --git a/document/src/vespa/document/select/valuenodes.cpp b/document/src/vespa/document/select/valuenodes.cpp index 1c7d47d0591..452779ca5ba 100644 --- a/document/src/vespa/document/select/valuenodes.cpp +++ b/document/src/vespa/document/select/valuenodes.cpp @@ -115,11 +115,28 @@ IntegerValueNode::print(std::ostream& out, bool verbose, if (hadParentheses()) out << ')'; } +void +BoolValueNode::visit(Visitor& visitor) const +{ + visitor.visitBoolValueNode(*this); +} + +void +BoolValueNode::print(std::ostream& out, + [[maybe_unused]] bool verbose, + [[maybe_unused]] const std::string& indent) const +{ + if (hadParentheses()) out << '('; + out << bool_value_str(); + if (hadParentheses()) out << ')'; +} + + int64_t CurrentTimeValueNode::getValue() const { struct timeval mytime; - gettimeofday(&mytime, 0); + gettimeofday(&mytime, nullptr); return mytime.tv_sec; } diff --git a/document/src/vespa/document/select/valuenodes.h b/document/src/vespa/document/select/valuenodes.h index f6fa0400d7c..0c4584e7eee 100644 --- a/document/src/vespa/document/select/valuenodes.h +++ b/document/src/vespa/document/select/valuenodes.h @@ -28,7 +28,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new InvalidValueNode(_name)); + return wrapParens(std::make_unique<InvalidValueNode>(_name)); } }; @@ -46,7 +46,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new NullValueNode()); + return wrapParens(std::make_unique<NullValueNode>()); } }; @@ -66,7 +66,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new StringValueNode(_value)); + return wrapParens(std::make_unique<StringValueNode>(_value)); } }; @@ -88,7 +88,28 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new IntegerValueNode(_value, _isBucketValue)); + return wrapParens(std::make_unique<IntegerValueNode>(_value, _isBucketValue)); + } +}; + +// Inherit from IntegerValueNode to be implicitly treated as an integer value by +// all code that does not explicitly know (or care) about bool values. +class BoolValueNode : public IntegerValueNode { +public: + explicit BoolValueNode(bool value) : IntegerValueNode(value, false) {} + + [[nodiscard]] bool bool_value() const noexcept { + return bool(getValue()); + } + [[nodiscard]] const char* bool_value_str() const noexcept { + return bool_value() ? "true" : "false"; + } + + void print(std::ostream& out, bool verbose, const std::string& indent) const override; + void visit(Visitor& visitor) const override; + + std::unique_ptr<ValueNode> clone() const override { + return wrapParens(std::make_unique<BoolValueNode>(bool_value())); } }; @@ -105,7 +126,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new CurrentTimeValueNode); + return wrapParens(std::make_unique<CurrentTimeValueNode>()); } }; @@ -123,7 +144,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new VariableValueNode(_value)); + return wrapParens(std::make_unique<VariableValueNode>(_value)); } }; @@ -131,7 +152,7 @@ class FloatValueNode : public ValueNode { double _value; public: - FloatValueNode(double val) : _value(val) {} + explicit FloatValueNode(double val) : _value(val) {} double getValue() const { return _value; } @@ -143,7 +164,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new FloatValueNode(_value)); + return wrapParens(std::make_unique<FloatValueNode>(_value)); } }; @@ -172,7 +193,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new FieldValueNode(_doctype, _fieldExpression)); + return wrapParens(std::make_unique<FieldValueNode>(_doctype, _fieldExpression)); } static vespalib::string extractFieldName(const vespalib::string & fieldExpression); @@ -229,10 +250,10 @@ private: ValueNode::UP clone() const override { if (_left_expr) { - return wrapParens(new FieldExprNode(std::unique_ptr<FieldExprNode>( + return wrapParens(std::make_unique<FieldExprNode>(std::unique_ptr<FieldExprNode>( static_cast<FieldExprNode*>(_left_expr->clone().release())), _right_expr)); } else { - return wrapParens(new FieldExprNode(_right_expr)); + return wrapParens(std::make_unique<FieldExprNode>(_right_expr)); } } }; @@ -261,7 +282,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new IdValueNode(_bucketIdFactory, _id, _typestring, _widthBits, _divisionBits)); + return wrapParens(std::make_unique<IdValueNode>(_bucketIdFactory, _id, _typestring, _widthBits, _divisionBits)); } int getWidthBits() const { return _widthBits; } @@ -298,7 +319,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new FunctionValueNode(_funcname, _source->clone())); + return wrapParens(std::make_unique<FunctionValueNode>(_funcname, _source->clone())); } const ValueNode& getChild() const { return *_source; } @@ -339,9 +360,7 @@ public: void visit(Visitor& visitor) const override; ValueNode::UP clone() const override { - return wrapParens(new ArithmeticValueNode(_left->clone(), - getOperatorName(), - _right->clone())); + return wrapParens(std::make_unique<ArithmeticValueNode>(_left->clone(), getOperatorName(), _right->clone())); } const ValueNode& getLeft() const { return *_left; } diff --git a/document/src/vespa/document/select/visitor.h b/document/src/vespa/document/select/visitor.h index a9b134f80fc..4664b831238 100644 --- a/document/src/vespa/document/select/visitor.h +++ b/document/src/vespa/document/select/visitor.h @@ -27,6 +27,7 @@ class InvalidConstant; class FieldValueNode; class FloatValueNode; class IntegerValueNode; +class BoolValueNode; class CurrentTimeValueNode; class StringValueNode; class NullValueNode; @@ -35,7 +36,7 @@ class VariableValueNode; class Visitor { public: - virtual ~Visitor() {} + virtual ~Visitor() = default; virtual void visitAndBranch(const And &) = 0; virtual void visitComparison(const Compare &) = 0; @@ -51,6 +52,7 @@ public: virtual void visitFloatValueNode(const FloatValueNode &) = 0; virtual void visitVariableValueNode(const VariableValueNode &) = 0; virtual void visitIntegerValueNode(const IntegerValueNode &) = 0; + virtual void visitBoolValueNode(const BoolValueNode&) = 0; virtual void visitCurrentTimeValueNode(const CurrentTimeValueNode &) = 0; virtual void visitStringValueNode(const StringValueNode &) = 0; virtual void visitNullValueNode(const NullValueNode &) = 0; diff --git a/searchcore/src/vespa/searchcore/proton/common/attributefieldvaluenode.cpp b/searchcore/src/vespa/searchcore/proton/common/attributefieldvaluenode.cpp index 87e9faa4fe2..8555af7c685 100644 --- a/searchcore/src/vespa/searchcore/proton/common/attributefieldvaluenode.cpp +++ b/searchcore/src/vespa/searchcore/proton/common/attributefieldvaluenode.cpp @@ -103,7 +103,7 @@ AttributeFieldValueNode::traceValue(const Context &context, std::ostream& out) c document::select::ValueNode::UP AttributeFieldValueNode::clone() const { - return wrapParens(new AttributeFieldValueNode(getDocType(), getFieldName(), _attr_guard_index)); + return wrapParens(std::make_unique<AttributeFieldValueNode>(getDocType(), getFieldName(), _attr_guard_index)); } } // namespace proton diff --git a/searchlib/src/tests/sortspec/multilevelsort.cpp b/searchlib/src/tests/sortspec/multilevelsort.cpp index 0154e95c155..9c1caaff662 100644 --- a/searchlib/src/tests/sortspec/multilevelsort.cpp +++ b/searchlib/src/tests/sortspec/multilevelsort.cpp @@ -9,6 +9,7 @@ #include <vespa/searchlib/uca/ucaconverter.h> #include <vespa/vespalib/util/testclock.h> #include <vespa/vespalib/testkit/testapp.h> +#include <type_traits> #include <vespa/log/log.h> LOG_SETUP("multilevelsort_test"); @@ -61,8 +62,8 @@ private: template<typename T> void fill(FloatingPointAttribute *attr, uint32_t size, uint32_t unique = 0); void fill(StringAttribute *attr, uint32_t size, const std::vector<std::string> &values); - template<typename T, typename V> - int compareTemplate(T *vector, uint32_t a, uint32_t b); + template <typename V> + int compareTemplate(AttributeVector *vector, uint32_t a, uint32_t b); int compare(AttributeVector *vector, AttrType type, uint32_t a, uint32_t b); void sortAndCheck(const std::vector<Spec> &spec, uint32_t num, uint32_t unique, const std::vector<std::string> &strValues); @@ -138,14 +139,23 @@ MultilevelSortTest::fill(StringAttribute *attr, uint32_t size, const std::vector } } -template<typename T, typename V> +template <typename V> +V get_helper(AttributeVector *vector, uint32_t doc_id) { + if constexpr (std::is_floating_point_v<V>) { + return vector->getFloat(doc_id); + } else { + return vector->getInt(doc_id); + } +} + +template <typename V> int -MultilevelSortTest::compareTemplate(T *vector, uint32_t a, uint32_t b) +MultilevelSortTest::compareTemplate(AttributeVector *vector, uint32_t a, uint32_t b) { V va; V vb; - vector->getAll(a, &va, 1); - vector->getAll(b, &vb, 1); + va = get_helper<V>(vector, a); + vb = get_helper<V>(vector, b); if (va == vb) { return 0; } else if (va < vb) { @@ -158,17 +168,17 @@ int MultilevelSortTest::compare(AttributeVector *vector, AttrType type, uint32_t a, uint32_t b) { if (type == INT8) { - return compareTemplate<Int8, int8_t>(static_cast<Int8*>(vector), a, b); + return compareTemplate<int8_t>(vector, a, b); } else if (type == INT16) { - return compareTemplate<Int16, int16_t>(static_cast<Int16*>(vector), a, b); + return compareTemplate<int16_t>(vector, a, b); } else if (type == INT32) { - return compareTemplate<Int32, int32_t>(static_cast<Int32*>(vector), a, b); + return compareTemplate<int32_t>(vector, a, b); } else if (type == INT64) { - return compareTemplate<Int64, int64_t>(static_cast<Int64*>(vector), a, b); + return compareTemplate<int64_t>(vector, a, b); } else if (type == FLOAT) { - return compareTemplate<Float, float>(static_cast<Float*>(vector), a, b); + return compareTemplate<float>(vector, a, b); } else if (type == DOUBLE) { - return compareTemplate<Double, double>(static_cast<Double*>(vector), a, b); + return compareTemplate<double>(vector, a, b); } else if (type == STRING) { StringAttribute *vString = static_cast<StringAttribute*>(vector); const char *va = vString->get(a); diff --git a/searchlib/src/vespa/searchlib/attribute/attrvector.h b/searchlib/src/vespa/searchlib/attribute/attrvector.h index 7713c210033..d1d2a1e8f3c 100644 --- a/searchlib/src/vespa/searchlib/attribute/attrvector.h +++ b/searchlib/src/vespa/searchlib/attribute/attrvector.h @@ -79,7 +79,6 @@ private: typedef typename B::WeightedFloat WeightedFloat; BaseType get(DocId doc) const override { return getHelper(doc, 0); } EnumHandle getEnum(DocId doc) const override { return getEnumHelper(doc, 0); } - uint32_t getAll(DocId doc, BaseType * v, uint32_t sz) const override { return getAllHelper<BaseType, BaseType>(doc, v, sz); } uint32_t get(DocId doc, EnumHandle * e, uint32_t sz) const override { return getAllEnumHelper(doc, e, sz); } uint32_t getValueCount(DocId doc) const override { return getValueCountHelper(doc); } @@ -125,7 +124,6 @@ private: } uint32_t get(DocId doc, WeightedEnum * v, uint32_t sz) const override { return getAllEnumHelper(doc, v, sz); } - uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const override { return getAllHelper<Weighted, BaseType>(doc, v, sz); } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { return getAllHelper<WeightedInt, largeint_t>(doc, v, sz); } uint32_t get(DocId doc, WeightedFloat * v, uint32_t sz) const override { return getAllHelper<WeightedFloat, double>(doc, v, sz); } }; diff --git a/searchlib/src/vespa/searchlib/attribute/floatbase.h b/searchlib/src/vespa/searchlib/attribute/floatbase.h index f10e8189c29..8ca6eda6421 100644 --- a/searchlib/src/vespa/searchlib/attribute/floatbase.h +++ b/searchlib/src/vespa/searchlib/attribute/floatbase.h @@ -50,8 +50,6 @@ class FloatingPointAttributeTemplate : public FloatingPointAttribute { public: using Weighted = WeightedType<T>; - virtual uint32_t getAll(DocId doc, T * v, uint32_t sz) const = 0; - virtual uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const = 0; protected: using EnumEntryType = T; using LoadedNumericValueT = attribute::LoadedNumericValue<T>; diff --git a/searchlib/src/vespa/searchlib/attribute/integerbase.h b/searchlib/src/vespa/searchlib/attribute/integerbase.h index 926ced7c7e0..65d16ce934a 100644 --- a/searchlib/src/vespa/searchlib/attribute/integerbase.h +++ b/searchlib/src/vespa/searchlib/attribute/integerbase.h @@ -49,8 +49,6 @@ class IntegerAttributeTemplate : public IntegerAttribute { public: using Weighted = WeightedType<T>; - virtual uint32_t getAll(DocId doc, T * v, uint32_t sz) const = 0; - virtual uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const = 0; protected: using EnumEntryType = T; using LoadedNumericValueT = attribute::LoadedNumericValue<T>; diff --git a/searchlib/src/vespa/searchlib/attribute/multinumericattribute.h b/searchlib/src/vespa/searchlib/attribute/multinumericattribute.h index e07498d9ca4..cc128b0eef1 100644 --- a/searchlib/src/vespa/searchlib/attribute/multinumericattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/multinumericattribute.h @@ -91,9 +91,6 @@ public: (void) doc; return std::numeric_limits<uint32_t>::max(); // does not have enum } - uint32_t getAll(DocId doc, T * v, uint32_t sz) const override { - return getHelper(doc, v, sz); - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { return getHelper(doc, v, sz); } @@ -125,9 +122,6 @@ public: } return available; } - uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const override{ - return getWeightedHelper<Weighted, T>(doc, v, sz); - } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { return getWeightedHelper<WeightedInt, largeint_t>(doc, v, sz); } diff --git a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h index 9f8506d3cb4..30e379d7739 100644 --- a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h @@ -77,9 +77,6 @@ public: } return valueCount; } - uint32_t getAll(DocId doc, T * v, uint32_t sz) const override { - return getHelper(doc, v, sz); - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { return getHelper(doc, v, sz); } @@ -96,9 +93,6 @@ public: } return valueCount; } - uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const override { - return getWeightedHelper<Weighted, T>(doc, v, sz); - } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { return getWeightedHelper<WeightedInt, largeint_t>(doc, v, sz); } diff --git a/searchlib/src/vespa/searchlib/attribute/singleboolattribute.h b/searchlib/src/vespa/searchlib/attribute/singleboolattribute.h index f7efe27277e..77c8b7e318f 100644 --- a/searchlib/src/vespa/searchlib/attribute/singleboolattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/singleboolattribute.h @@ -45,12 +45,6 @@ public: uint32_t getEnum(DocId) const override { return std::numeric_limits<uint32_t>::max(); // does not have enum } - uint32_t getAll(DocId doc, int8_t * v, uint32_t sz) const override { - if (sz > 0) { - v[0] = getFast(doc); - } - return 1; - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { if (sz > 0) { v[0] = static_cast<largeint_t>(getFast(doc)); @@ -69,7 +63,6 @@ public: } return 1; } - uint32_t getAll(DocId, Weighted *, uint32_t) const override { return 0; } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { if (sz > 0) { v[0] = WeightedInt(static_cast<largeint_t>(getFast(doc))); diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericattribute.h b/searchlib/src/vespa/searchlib/attribute/singlenumericattribute.h index 71a5f4f738e..cf3fa85a060 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlenumericattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/singlenumericattribute.h @@ -93,11 +93,6 @@ public: (void) doc; return std::numeric_limits<uint32_t>::max(); // does not have enum } - uint32_t getAll(DocId doc, T * v, uint32_t sz) const override { - (void) sz; - v[0] = getFast(doc); - return 1; - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { (void) sz; v[0] = static_cast<largeint_t>(getFast(doc)); @@ -113,10 +108,6 @@ public: e[0] = getEnum(doc); return 1; } - uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const override { - (void) doc; (void) v; (void) sz; - return 0; - } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { (void) sz; v[0] = WeightedInt(static_cast<largeint_t>(getFast(doc))); diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h index a269aec5c6b..5b0e1c6131e 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/singlenumericenumattribute.h @@ -74,12 +74,6 @@ public: double getFloat(DocId doc) const override { return static_cast<double>(get(doc)); } - uint32_t getAll(DocId doc, T * v, uint32_t sz) const override { - if (sz > 0) { - v[0] = get(doc); - } - return 1; - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { if (sz > 0) { v[0] = getInt(doc); @@ -92,12 +86,6 @@ public: } return 1; } - uint32_t getAll(DocId doc, Weighted * v, uint32_t sz) const override { - if (sz > 0) { - v[0] = Weighted(get(doc)); - } - return 1; - } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { if (sz > 0) { v[0] = WeightedInt(getInt(doc)); diff --git a/searchlib/src/vespa/searchlib/attribute/singlesmallnumericattribute.h b/searchlib/src/vespa/searchlib/attribute/singlesmallnumericattribute.h index f6059d3d510..646edc786a3 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlesmallnumericattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/singlesmallnumericattribute.h @@ -100,12 +100,6 @@ public: uint32_t getEnum(DocId) const override { return std::numeric_limits<uint32_t>::max(); // does not have enum } - uint32_t getAll(DocId doc, T * v, uint32_t sz) const override { - if (sz > 0) { - v[0] = getFast(doc); - } - return 1; - } uint32_t get(DocId doc, largeint_t * v, uint32_t sz) const override { if (sz > 0) { v[0] = static_cast<largeint_t>(getFast(doc)); @@ -124,7 +118,6 @@ public: } return 1; } - uint32_t getAll(DocId, Weighted *, uint32_t) const override { return 0; } uint32_t get(DocId doc, WeightedInt * v, uint32_t sz) const override { if (sz > 0) { v[0] = WeightedInt(static_cast<largeint_t>(getFast(doc))); diff --git a/storage/src/vespa/storage/persistence/fieldvisitor.h b/storage/src/vespa/storage/persistence/fieldvisitor.h index 3216a360c82..93782d3fbe2 100644 --- a/storage/src/vespa/storage/persistence/fieldvisitor.h +++ b/storage/src/vespa/storage/persistence/fieldvisitor.h @@ -45,6 +45,7 @@ public: void visitFloatValueNode(const document::select::FloatValueNode &) override {} void visitVariableValueNode(const document::select::VariableValueNode &) override {} void visitIntegerValueNode(const document::select::IntegerValueNode &) override {} + void visitBoolValueNode(const document::select::BoolValueNode &) override {} void visitCurrentTimeValueNode(const document::select::CurrentTimeValueNode &) override {} void visitStringValueNode(const document::select::StringValueNode &) override {} void visitNullValueNode(const document::select::NullValueNode &) override {} |