Merge pull request #27765 from vespa-engine/balder/refactor-query-building-1

Balder/refactor query building 1

6 files changed, 72 insertions, 35 deletions

diff --git a/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp b/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp
index b005a2a6c7d..ba791444dea 100644
--- a/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp
@@ -66,6 +66,7 @@ using search::query::StackDumpCreator;
 using search::query::StringTerm;
 using search::query::SubstringTerm;
 using search::query::SuffixTerm;
+using search::query::Term;
 using search::queryeval::AndBlueprint;
 using search::queryeval::AndSearchStrict;
 using search::queryeval::Blueprint;
@@ -687,16 +688,20 @@ public:
     ~CreateBlueprintVisitor() override;
 
     template <class TermNode>
-    void visitTerm(TermNode &n, bool simple = false) {
-        if (simple && (_dwa != nullptr) && !_field.isFilter() && n.isRanked()) {
+    void visitSimpleTerm(TermNode &n) {
+        if ((_dwa != nullptr) && !_field.isFilter() && n.isRanked() && !Term::isPossibleRangeTerm(n.getTerm())) {
             NodeAsKey key(n, _scratchPad);
             setResult(std::make_unique<DirectAttributeBlueprint>(_field, _attr, *_dwa, key));
         } else {
-            SearchContextParams scParams = createContextParams(_field.isFilter());
-            const string stack = StackDumpCreator::create(n);
-            setResult(std::make_unique<AttributeFieldBlueprint>(_field, _attr, stack, scParams));
+            visitTerm(n);
         }
     }
+    template <class TermNode>
+    void visitTerm(TermNode &n) {
+        SearchContextParams scParams = createContextParams(_field.isFilter());
+        const string stack = StackDumpCreator::create(n);
+        setResult(std::make_unique<AttributeFieldBlueprint>(_field, _attr, stack, scParams));
+    }
 
     void visitLocation(LocationTerm &node) {
         setResult(make_location_blueprint(_field, _attr, node.getTerm(), createContextParams(_field.isFilter())));
@@ -712,7 +717,7 @@ public:
         }
     }
 
-    void visit(NumberTerm & n) override { visitTerm(n, true); }
+    void visit(NumberTerm & n) override { visitSimpleTerm(n); }
     void visit(LocationTerm &n) override { visitLocation(n); }
     void visit(PrefixTerm & n) override { visitTerm(n); }
 
@@ -736,7 +741,7 @@ public:
         }
     }
 
-    void visit(StringTerm & n) override { visitTerm(n, true); }
+    void visit(StringTerm & n) override { visitSimpleTerm(n); }
     void visit(SubstringTerm & n) override {
         query::SimpleRegExpTerm re(vespalib::RegexpUtil::make_from_substring(n.getTerm()),
                                    n.getView(), n.getId(), n.getWeight());
diff --git a/searchlib/src/vespa/searchlib/query/tree/CMakeLists.txt b/searchlib/src/vespa/searchlib/query/tree/CMakeLists.txt
index 4fcec251caa..a3a1d52701e 100644
--- a/searchlib/src/vespa/searchlib/query/tree/CMakeLists.txt
+++ b/searchlib/src/vespa/searchlib/query/tree/CMakeLists.txt
@@ -4,12 +4,13 @@ vespa_add_library(searchlib_query_tree OBJECT
     const_bool_nodes.cpp
     intermediate.cpp
     intermediatenodes.cpp
+    location.cpp
     querybuilder.cpp
+    range.cpp
     simplequery.cpp
     stackdumpcreator.cpp
+    stackdumpquerycreator.cpp
     term.cpp
-    location.cpp
-    range.cpp
     termnodes.cpp
     DEPENDS
 )
diff --git a/searchlib/src/vespa/searchlib/query/tree/querybuilder.h b/searchlib/src/vespa/searchlib/query/tree/querybuilder.h
index 979f48fec89..a2c3f4a5af4 100644
--- a/searchlib/src/vespa/searchlib/query/tree/querybuilder.h
+++ b/searchlib/src/vespa/searchlib/query/tree/querybuilder.h
@@ -82,7 +82,7 @@ public:
     /**
      * If build failed, the reason is stored here.
      */
-    vespalib::string error() { return _error_msg; }
+    vespalib::string error() const { return _error_msg; }
 
     /**
      * After an error, reset() must be called before attempting to
diff --git a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.cpp b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.cpp
new file mode 100644
index 00000000000..c4c99edd73b
--- /dev/null
+++ b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.cpp
@@ -0,0 +1,43 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "stackdumpquerycreator.h"
+#include <vespa/vespalib/objects/hexdump.h>
+
+#include <vespa/log/log.h>
+LOG_SETUP(".searchlib.query.tree.stackdumpquerycreator");
+
+using vespalib::Issue;
+
+namespace search::query {
+
+void
+StackDumpQueryCreatorHelper::populateMultiTerm(SimpleQueryStackDumpIterator &queryStack, QueryBuilderBase & builder, MultiTerm & mt) {
+    uint32_t added(0);
+    for (added = 0; (added < mt.getNumTerms()) && queryStack.next(); added++) {
+        ParseItem::ItemType type = queryStack.getType();
+        switch (type) {
+            case ParseItem::ITEM_PURE_WEIGHTED_LONG:
+                mt.addTerm(queryStack.getIntergerTerm(), queryStack.GetWeight());
+                break;
+            case ParseItem::ITEM_PURE_WEIGHTED_STRING:
+                mt.addTerm(queryStack.getTerm(), queryStack.GetWeight());
+                break;
+            default:
+                builder.reportError(vespalib::make_string("Got unexpected node %d for multiterm node at child term %d", type, added));
+                return;
+        }
+    }
+    if (added < mt.getNumTerms()) {
+        builder.reportError(vespalib::make_string("Too few nodes(%d) for multiterm(%d)", added, mt.getNumTerms()));
+    }
+}
+
+void
+StackDumpQueryCreatorHelper::reportError(const SimpleQueryStackDumpIterator &queryStack, const QueryBuilderBase & builder) {
+    vespalib::stringref stack = queryStack.getStack();
+    Issue::report("Unable to create query tree from stack dump. Failed at position %ld out of %ld bytes %s",
+               queryStack.getPosition(), stack.size(), builder.error().c_str());
+    LOG(error, "got bad query stack: %s", vespalib::HexDump(stack.data(), stack.size()).toString().c_str());
+}
+
+}
diff --git a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
index a552a650704..160fe747db0 100644
--- a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
+++ b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
@@ -4,16 +4,21 @@
 
 #include "node.h"
 #include "querybuilder.h"
-#include "term.h"
+#include "termnodes.h"
 #include <vespa/searchlib/parsequery/stackdumpiterator.h>
 #include <vespa/searchlib/common/geo_location_parser.h>
-#include <vespa/vespalib/objects/hexdump.h>
 #include <vespa/vespalib/util/stringfmt.h>
 #include <vespa/vespalib/util/issue.h>
 #include <charconv>
 
 namespace search::query {
 
+class StackDumpQueryCreatorHelper {
+public:
+    static void populateMultiTerm(SimpleQueryStackDumpIterator &queryStack, QueryBuilderBase & builder, MultiTerm & mt);
+    static void reportError(const SimpleQueryStackDumpIterator &queryStack, const QueryBuilderBase & builder);
+};
+
 /**
  * Creates a query tree from a stack dump.
  */
@@ -40,34 +45,14 @@ public:
             }
         }
         if (builder.hasError()) {
-            vespalib::stringref stack = queryStack.getStack();
-            vespalib::Issue::report("Unable to create query tree from stack dump. Failed at position %ld out of %ld bytes %s",
-                       queryStack.getPosition(), stack.size(), builder.error().c_str());
-            LOG(error, "got bad query stack: %s", vespalib::HexDump(stack.data(), stack.size()).toString().c_str());
+            StackDumpQueryCreatorHelper::reportError(queryStack, builder);
         }
         return builder.build();
     }
 
 private:
     static void populateMultiTerm(search::SimpleQueryStackDumpIterator &queryStack, QueryBuilderBase & builder, MultiTerm & mt) {
-        uint32_t added(0);
-        for (added = 0; (added < mt.getNumTerms()) && queryStack.next(); added++) {
-            ParseItem::ItemType type = queryStack.getType();
-            switch (type) {
-                case ParseItem::ITEM_PURE_WEIGHTED_LONG:
-                    mt.addTerm(queryStack.getIntergerTerm(), queryStack.GetWeight());
-                    break;
-                case ParseItem::ITEM_PURE_WEIGHTED_STRING:
-                    mt.addTerm(queryStack.getTerm(), queryStack.GetWeight());
-                    break;
-                default:
-                    builder.reportError(vespalib::make_string("Got unexpected node %d for multiterm node at child term %d", type, added));
-                    return;
-            }
-        }
-        if (added < mt.getNumTerms()) {
-            builder.reportError(vespalib::make_string("Too few nodes(%d) for multiterm(%d)", added, mt.getNumTerms()));
-        }
+        StackDumpQueryCreatorHelper::populateMultiTerm(queryStack, builder, mt);
     }
     static Term *
     createQueryTerm(search::SimpleQueryStackDumpIterator &queryStack, QueryBuilder<NodeTypes> & builder, vespalib::stringref & pureTermView) {
@@ -189,7 +174,7 @@ private:
                 Location loc(parser.getGeoLocation());
                 t = &builder.addLocationTerm(loc, view, id, weight);
             } else if (type == ParseItem::ITEM_NUMTERM) {
-                if (term[0] == '[' || term[0] == '<' || term[0] == '>') {
+                if (Term::isPossibleRangeTerm(term)) {
                     Range range(term);
                     t = &builder.addRangeTerm(range, view, id, weight);
                 } else {
diff --git a/searchlib/src/vespa/searchlib/query/tree/term.h b/searchlib/src/vespa/searchlib/query/tree/term.h
index b8ce4fe035d..749d0f5a588 100644
--- a/searchlib/src/vespa/searchlib/query/tree/term.h
+++ b/searchlib/src/vespa/searchlib/query/tree/term.h
@@ -34,6 +34,9 @@ public:
     bool isRanked() const { return _ranked; }
     bool usePositionData() const { return _position_data; }
 
+    static bool isPossibleRangeTerm(vespalib::stringref term) noexcept {
+        return (term[0] == '[' || term[0] == '<' || term[0] == '>');
+    }
 protected:
     Term(vespalib::stringref view, int32_t id, Weight weight);
 };