Merge pull request #26727 from vespa-engine/balder/ensure-we-always-reference-a-valid-empty-string

Balder/ensure we always reference a valid empty string

9 files changed, 54 insertions, 45 deletions

diff --git a/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp b/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
index a19b093ef4d..2804e3f74e4 100644
--- a/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
+++ b/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
@@ -8,7 +8,6 @@
 
 #include <vespa/searchlib/attribute/enumstore.hpp>
 #include <vespa/searchlib/attribute/single_string_enum_search_context.h>
-#include <vespa/searchlib/attribute/singlestringpostattribute.h>
 #include <vespa/searchlib/attribute/multistringpostattribute.hpp>
 
 #include <vespa/log/log.h>
diff --git a/searchlib/src/tests/expression/attributenode/attribute_node_test.cpp b/searchlib/src/tests/expression/attributenode/attribute_node_test.cpp
index 896fd27f90e..757f74bdaff 100644
--- a/searchlib/src/tests/expression/attributenode/attribute_node_test.cpp
+++ b/searchlib/src/tests/expression/attributenode/attribute_node_test.cpp
@@ -54,8 +54,7 @@ namespace {
 vespalib::string stringValue(const ResultNode &result, const IAttributeVector &attr) {
     if (result.inherits(EnumResultNode::classId)) {
         auto enumHandle = result.getEnum();
-        auto &stringAttr = dynamic_cast<const StringAttribute &>(attr);
-        return vespalib::string(stringAttr.getFromEnum(enumHandle));
+        return vespalib::string(attr.getStringFromEnum(enumHandle));
     }
     char buf[100];
     BufferRef bref(&buf[0], sizeof(buf));
@@ -199,8 +198,9 @@ AttributeManagerFixture::buildIntegerArrayAttribute(const vespalib::string &name
 }
 
 
-struct Fixture
+class Fixture
 {
+public:
     AttributeManagerFixture             attrs;
     AttributeContext                    context;
     Fixture();
@@ -208,11 +208,13 @@ struct Fixture
     std::unique_ptr<AttributeNode> makeNode(const vespalib::string &attributeName, bool useEnumOptimiation = false, bool preserveAccurateTypes = false);
     void assertInts(std::vector<IAttributeVector::largeint_t> expVals, const vespalib::string &attributteName, bool preserveAccurateTypes = false);
     void assertBools(std::vector<bool> expVals, const vespalib::string &attributteName, bool preserveAccurateTypes = false);
-    void assertStrings(std::vector<vespalib::string> expVals, const vespalib::string &attributteName, bool useEnumOptimization = false);
+    void assertStrings(std::vector<vespalib::string> expVals, const vespalib::string &attributteName);
     void assertFloats(std::vector<double> expVals, const vespalib::string &attributteName);
     void assertIntArrays(std::vector<std::vector<IAttributeVector::largeint_t>> expVals, const vespalib::string &attributteName, bool preserveAccurateTypes = false);
     void assertStringArrays(std::vector<std::vector<vespalib::string>> expVals, const vespalib::string &attributteName, bool useEnumOptimization = false);
     void assertFloatArrays(std::vector<std::vector<double>> expVals, const vespalib::string &attributteName);
+private:
+    void assertStrings(std::vector<vespalib::string> expVals, const vespalib::string &attributteName, bool useEnumOptimization);
 };
 
 Fixture::Fixture()
@@ -280,6 +282,11 @@ Fixture::assertBools(std::vector<bool> expVals, const vespalib::string &attribut
     }
 }
 
+void
+Fixture::assertStrings(std::vector<vespalib::string> expVals, const vespalib::string &attributeName) {
+    assertStrings(expVals, attributeName, false);
+    assertStrings(expVals, attributeName, true);
+}
 
 void
 Fixture::assertStrings(std::vector<vespalib::string> expVals, const vespalib::string &attributeName, bool useEnumOptimization)
@@ -293,6 +300,9 @@ Fixture::assertStrings(std::vector<vespalib::string> expVals, const vespalib::st
         const auto &result = *node->getResult();
         if (useEnumOptimization) {
             ASSERT_TRUE(result.inherits(EnumResultNode::classId));
+            search::enumstore::EnumHandle enumVal(0);
+            ASSERT_TRUE(node->getAttribute()->findEnum(expDocVal.c_str(), enumVal));
+            EXPECT_EQUAL(result.getEnum(), enumVal);
         } else {
             ASSERT_TRUE(result.inherits(StringResultNode::classId));
         }
@@ -404,7 +414,6 @@ TEST_F("test single values", Fixture)
     TEST_DO(f.assertInts({ 10, getUndefined<int8_t>()}, "ifield"));
     TEST_DO(f.assertInts({ 10, getUndefined<int8_t>()}, "ifield", true));
     TEST_DO(f.assertStrings({ "n1", "" }, "sfield"));
-    TEST_DO(f.assertStrings({ "n1", "" }, "sfield", true));
     TEST_DO(f.assertFloats({ 110.0, getUndefined<double>() }, "ffield"));
 }
 
diff --git a/searchlib/src/tests/features/prod_features.cpp b/searchlib/src/tests/features/prod_features.cpp
index 4ebc94ccb8b..dc64c3328e4 100644
--- a/searchlib/src/tests/features/prod_features.cpp
+++ b/searchlib/src/tests/features/prod_features.cpp
@@ -1252,7 +1252,7 @@ Test::testDotProduct()
             { // test funky syntax
                 dotproduct::wset::EnumVector out(sv);
                 WeightedSetParser::parse("( a: 1,  b:2 ,c: , :3)", out);
-                EXPECT_EQUAL(out.getVector().size(), 3u);
+                EXPECT_EQUAL(out.getVector().size(), 4u);
                 EXPECT_TRUE(sv->findEnum("a", e));
                 EXPECT_EQUAL(out.getVector()[0].first, e);
                 EXPECT_EQUAL(out.getVector()[0].second, 1);
@@ -1262,6 +1262,9 @@ Test::testDotProduct()
                 EXPECT_TRUE(sv->findEnum("c", e));
                 EXPECT_EQUAL(out.getVector()[2].first, e);
                 EXPECT_EQUAL(out.getVector()[2].second, 0);
+                EXPECT_TRUE(sv->findEnum("", e));
+                EXPECT_EQUAL(out.getVector()[3].first, e);
+                EXPECT_EQUAL(out.getVector()[3].second, 3);
             }
             { // strings not in attribute vector
                 dotproduct::wset::EnumVector out(sv);
diff --git a/searchlib/src/vespa/searchlib/attribute/attributevector.cpp b/searchlib/src/vespa/searchlib/attribute/attributevector.cpp
index e6b584d29b2..f0c8d9a2191 100644
--- a/searchlib/src/vespa/searchlib/attribute/attributevector.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/attributevector.cpp
@@ -441,20 +441,20 @@ AttributeVector::addReservedDoc()
     assert(docId == 0u);
     assert(docId < getNumDocs());
     clearDoc(docId);
-    commit();
-    FloatingPointAttribute * vec = dynamic_cast<FloatingPointAttribute *>(this);
-    if (vec) {
-        if (hasMultiValue()) {
+    if (hasMultiValue()) {
+        if (isFloatingPointType()) {
+            auto * vec = dynamic_cast<FloatingPointAttribute *>(this);
             bool appendedUndefined = vec->append(0, attribute::getUndefined<double>(), 1);
             assert(appendedUndefined);
             (void) appendedUndefined;
-        } else {
-            bool updatedUndefined = vec->update(0, attribute::getUndefined<double>());
-            assert(updatedUndefined);
-            (void) updatedUndefined;
+        } else if (isStringType()) {
+            auto * vec = dynamic_cast<StringAttribute *>(this);
+            bool appendedUndefined = vec->append(0, StringAttribute::defaultValue(), 1);
+            assert(appendedUndefined);
+            (void) appendedUndefined;
         }
-        commit();
     }
+    commit();
 }
 
 attribute::IPostingListAttributeBase *AttributeVector::getIPostingListAttributeBase() { return nullptr; }
diff --git a/searchlib/src/vespa/searchlib/attribute/stringbase.h b/searchlib/src/vespa/searchlib/attribute/stringbase.h
index ed8777d6bab..5c6bf3c6b6a 100644
--- a/searchlib/src/vespa/searchlib/attribute/stringbase.h
+++ b/searchlib/src/vespa/searchlib/attribute/stringbase.h
@@ -53,11 +53,11 @@ public:
     largeint_t getInt(DocId doc)  const override { return strtoll(get(doc), nullptr, 0); }
     double getFloat(DocId doc)    const override;
     vespalib::ConstArrayRef<char> get_raw(DocId) const override;
+    static const char * defaultValue() { return ""; }
 protected:
     StringAttribute(const vespalib::string & name);
     StringAttribute(const vespalib::string & name, const Config & c);
     ~StringAttribute() override;
-    static const char * defaultValue() { return ""; }
     using Change = ChangeTemplate<StringChangeData>;
     using ChangeVector = ChangeVectorT<Change>;
     using EnumEntryType = const char*;
diff --git a/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp b/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp
index b3cc126efdb..52ee467fac8 100644
--- a/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp
+++ b/searchlib/src/vespa/searchlib/expression/attribute_map_lookup_node.cpp
@@ -1,11 +1,11 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "attribute_map_lookup_node.h"
-#include <vespa/vespalib/stllike/asciistream.h>
-#include <vespa/vespalib/util/exceptions.h>
+#include <vespa/searchlib/attribute/stringbase.h>
 #include <vespa/searchcommon/attribute/attributecontent.h>
 #include <vespa/searchcommon/attribute/iattributecontext.h>
-#include <vespa/searchcommon/common/undefinedvalues.h>
+#include <vespa/vespalib/stllike/asciistream.h>
+#include <vespa/vespalib/util/exceptions.h>
 
 using search::attribute::AttributeContent;
 using search::attribute::IAttributeVector;
@@ -36,8 +36,6 @@ public:
 
 namespace {
 
-vespalib::string indirectKeyMarker("attribute(");
-
 class BadKeyHandler : public AttributeMapLookupNode::KeyHandler
 {
 public:
@@ -220,7 +218,6 @@ IAttributeVector::largeint_t getUndefinedValue(BasicType::Type basicType)
         return getUndefined<int32_t>();
     case BasicType::INT64:
         return getUndefined<int64_t>();
-        break;
     default:
         return 0;
     }
@@ -330,7 +327,6 @@ AttributeMapLookupNode::onPrepare(bool preserveAccurateTypes)
                     break;
                 default:
                     throw std::runtime_error("This is no valid integer attribute " + attribute->getName());
-                    break;
                 }
             } else {
                 prepareIntValues<Int64ResultNode>(std::move(keyHandler), *attribute, undefinedValue);
@@ -342,7 +338,11 @@ AttributeMapLookupNode::onPrepare(bool preserveAccurateTypes)
         } else if (attribute->isStringType()) {
             if (_useEnumOptimization) {
                 auto resultNode = std::make_unique<EnumResultNode>();
-                _handler = std::make_unique<EnumValueHandler>(std::move(keyHandler), *attribute, *resultNode, EnumHandle());
+                const StringAttribute & sattr = dynamic_cast<const StringAttribute &>(*attribute);
+                EnumHandle undefined(0);
+                bool found = attribute->findEnum(sattr.defaultValue(), undefined);
+                assert(found);
+                _handler = std::make_unique<EnumValueHandler>(std::move(keyHandler), *attribute, *resultNode, undefined);
                 setResultType(std::move(resultNode));
             } else {
                 auto resultNode = std::make_unique<StringResultNode>();
diff --git a/searchlib/src/vespa/searchlib/queryeval/wand/parallel_weak_and_search.cpp b/searchlib/src/vespa/searchlib/queryeval/wand/parallel_weak_and_search.cpp
index 292605127fb..1a7e91b2d1a 100644
--- a/searchlib/src/vespa/searchlib/queryeval/wand/parallel_weak_and_search.cpp
+++ b/searchlib/src/vespa/searchlib/queryeval/wand/parallel_weak_and_search.cpp
@@ -136,27 +136,25 @@ createWand(const wand::Terms &terms,
     using WandType = ParallelWeakAndSearchImpl<VectorizedIteratorTerms, FutureHeap, PastHeap, IS_STRICT>;
     if (should_monitor_wand()) {
         wand::Terms termsWithMonitoring = insertMonitoringSearchIterator(terms);
-        MonitoringSearchIterator::UP monitoringIterator =
-            MonitoringSearchIterator::UP(new MonitoringSearchIterator
-            (make_string("PWAND(%u,%" PRId64 "),strict=%u",
-                         matchParams.scores.getScoresToTrack(),
-                         matchParams.scoreThreshold,
-                         IS_STRICT),
-             SearchIterator::UP(new WandType(rankParams.rootMatchData,
-                                             VectorizedIteratorTerms(termsWithMonitoring,
-                                                     DotProductScorer(),
-                                                     matchParams.docIdLimit,
-                                                     std::move(rankParams.childrenMatchData)),
-                                             matchParams)),
-             false));
+        auto monitoringIterator = std::make_unique<MonitoringSearchIterator>(
+                make_string("PWAND(%u,%" PRId64 "),strict=%u",
+                            matchParams.scores.getScoresToTrack(),
+                            matchParams.scoreThreshold, IS_STRICT),
+                std::make_unique<WandType>(rankParams.rootMatchData,
+                                           VectorizedIteratorTerms(termsWithMonitoring,
+                                                                   DotProductScorer(),
+                                                                   matchParams.docIdLimit,
+                                                                   std::move(rankParams.childrenMatchData)),
+                                           matchParams),
+                false);
         return std::make_unique<MonitoringDumpIterator>(std::move(monitoringIterator));
     }
     return std::make_unique<WandType>(rankParams.rootMatchData,
-                        VectorizedIteratorTerms(terms,
-                                DotProductScorer(),
-                                matchParams.docIdLimit,
-                                std::move(rankParams.childrenMatchData)),
-                        matchParams);
+                                      VectorizedIteratorTerms(terms,
+                                                              DotProductScorer(),
+                                                              matchParams.docIdLimit,
+                                                              std::move(rankParams.childrenMatchData)),
+                                      matchParams);
 }
 
 } // namespace search::queryeval::wand::<unnamed>
diff --git a/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.cpp b/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.cpp
index a030dcb0640..2edd00ad5a5 100644
--- a/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.cpp
+++ b/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.cpp
@@ -13,7 +13,7 @@ vespalib::string indirectKeyMarker("attribute(");
 }
 
 std::unique_ptr<AttributeNode>
-makeAttributeMapLookupNode(const vespalib::string attributeName)
+makeAttributeMapLookupNode(vespalib::stringref attributeName)
 {
     vespalib::asciistream keyName;
     vespalib::asciistream valueName;
diff --git a/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.h b/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.h
index f56cea962d3..baa074b4004 100644
--- a/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.h
+++ b/searchlib/src/vespa/searchlib/test/make_attribute_map_lookup_node.h
@@ -9,6 +9,6 @@ namespace search::expression { class AttributeNode; }
 namespace search::expression::test {
 
 std::unique_ptr<AttributeNode>
-makeAttributeMapLookupNode(const vespalib::string attributeName);
+makeAttributeMapLookupNode(vespalib::stringref attributeName);
 
 }