Merge pull request #6191 from vespa-engine/toregge/detect-unchanged-field-in-struct

Handle struct field attributes in attribute reprocessing initializer and attribute aspect delayer.

7 files changed, 195 insertions, 49 deletions

diff --git a/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp b/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp
index 3ef2461c682..aeafcc26794 100644
--- a/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp
+++ b/searchcore/src/tests/proton/attribute/attribute_aspect_delayer/attribute_aspect_delayer_test.cpp
@@ -36,15 +36,24 @@ namespace proton
 
 namespace {
 
-AttributesConfig::Attribute make_sv_cfg(AttributesConfig::Attribute::Datatype dataType)
+AttributesConfig::Attribute make_sv_cfg(const vespalib::string &name, AttributesConfig::Attribute::Datatype dataType)
 {
     AttributesConfig::Attribute attr;
-    attr.name = "a";
+    attr.name = name;
     attr.datatype = dataType;
     attr.collectiontype = AttributesConfig::Attribute::Collectiontype::SINGLE;
     return attr;
 }
 
+AttributesConfig::Attribute make_sv_cfg(AttributesConfig::Attribute::Datatype dataType)
+{
+    return make_sv_cfg("a", dataType);
+}
+
+AttributesConfig::Attribute make_int32_sv_cfg(const vespalib::string &name) {
+    return make_sv_cfg(name, AttributesConfig::Attribute::Datatype::INT32);
+}
+
 AttributesConfig::Attribute make_int32_sv_cfg() {
     return make_sv_cfg(AttributesConfig::Attribute::Datatype::INT32);
 }
@@ -104,6 +113,14 @@ SummarymapConfig::Override make_geopos_override(const vespalib::string &name)
     return override;
 }
 
+SummarymapConfig::Override make_attribute_combiner_override(const vespalib::string &name)
+{
+    SummarymapConfig::Override override;
+    override.field = name;
+    override.command = "attributecombiner";
+    return override;
+}
+
 SummarymapConfig smCfg(std::vector<SummarymapConfig::Override> overrides)
 {
     SummarymapConfigBuilder result;
@@ -306,6 +323,29 @@ TEST_F("require that fast access flag change is not delayed, true->false edge, s
     TEST_DO(f.assertSummarymapConfig({make_attribute_override("a")}));
 }
 
+TEST_F("require that adding attribute aspect to struct field is not delayed if field type is changed", Fixture)
+{
+    f.setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), smCfg({make_attribute_combiner_override("array")}));
+    TEST_DO(f.assertAttributeConfig({make_int32_sv_cfg("array.a")}));
+    TEST_DO(f.assertSummarymapConfig({make_attribute_combiner_override("array")}));
+}
+
+TEST_F("require that adding attribute aspect to struct field is delayed if field type is unchanged", Fixture)
+{
+    f.addFields({"array.a"});
+    f.setup(attrCfg({}), smCfg({}), attrCfg({make_int32_sv_cfg("array.a")}), smCfg({make_attribute_combiner_override("array")}));
+    TEST_DO(f.assertAttributeConfig({}));
+    TEST_DO(f.assertSummarymapConfig({}));
+}
+
+TEST_F("require that removing attribute aspect from struct field is not delayed", Fixture)
+{
+    f.addFields({"array.a"});
+    f.setup(attrCfg({make_int32_sv_cfg("array.a")}), smCfg({make_attribute_combiner_override("array")}), attrCfg({}), smCfg({}));
+    TEST_DO(f.assertAttributeConfig({}));
+    TEST_DO(f.assertSummarymapConfig({}));
+}
+
 }
 
 TEST_MAIN()
diff --git a/searchcore/src/tests/proton/common/document_type_inspector/document_type_inspector_test.cpp b/searchcore/src/tests/proton/common/document_type_inspector/document_type_inspector_test.cpp
index 2dbff7d40dd..167865b5c68 100644
--- a/searchcore/src/tests/proton/common/document_type_inspector/document_type_inspector_test.cpp
+++ b/searchcore/src/tests/proton/common/document_type_inspector/document_type_inspector_test.cpp
@@ -1,78 +1,131 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #include <vespa/searchcore/proton/common/document_type_inspector.h>
 #include <vespa/vespalib/testkit/testapp.h>
-#include <vespa/document/repo/configbuilder.h>
-#include <vespa/document/repo/documenttyperepo.h>
+#include <vespa/document/base/field.h>
+#include <vespa/document/datatype/datatypes.h>
 
 using namespace document;
 using namespace proton;
-using document::config_builder::DocumenttypesConfigBuilderHelper;
-using document::config_builder::Struct;
 
-const int32_t doc_type_id = 787121340;
-const vespalib::string type_name = "test";
-const vespalib::string header_name = type_name + ".header";
-const vespalib::string body_name = type_name + ".body";
-
-std::unique_ptr<const DocumentTypeRepo>
-makeOldDocTypeRepo()
+template <class Type>
+void
+addFields(Type &type, bool fieldF3IsString, bool hasFieldF4, bool hasFieldF5)
 {
-    DocumenttypesConfigBuilderHelper builder;
-    builder.document(doc_type_id, type_name,
-                     Struct(header_name), Struct(body_name).
-                     addField("f1", DataType::T_STRING).
-                     addField("f2", DataType::T_STRING).
-                     addField("f3", DataType::T_STRING).
-                     addField("f4", DataType::T_STRING));
-    return std::unique_ptr<const DocumentTypeRepo>(new DocumentTypeRepo(builder.config()));
+    type.addField(Field("f1", 1, *DataType::STRING, true));
+    type.addField(Field("f2", 2, *DataType::STRING, true));
+    type.addField(Field("f3", 3, fieldF3IsString ? *DataType::STRING : *DataType::INT, true));
+    if (hasFieldF4) {
+        type.addField(Field("f4", 4, *DataType::STRING, true));
+    }
+    if (hasFieldF5) {
+        type.addField(Field("f5", 5, *DataType::STRING, true));
+    }
 }
 
-std::unique_ptr<const DocumentTypeRepo>
-makeNewDocTypeRepo()
+struct DocumentTypeFixture
+{
+    DocumentType _documentType;
+    StructDataType _structFieldType;
+    ArrayDataType _structArrayFieldType;
+    MapDataType _structMapFieldType;
+    MapDataType _mapFieldType;
+
+    DocumentTypeFixture(bool fieldF3IsString, bool hasFieldF4, bool hasFieldF5, bool hasStruct, bool mapKeyIsByte);
+    ~DocumentTypeFixture();
+};
+
+DocumentTypeFixture::DocumentTypeFixture(bool fieldF3IsString, bool hasFieldF4, bool hasFieldF5, bool hasStruct, bool mapKeyIsByte)
+    : _documentType("test"),
+      _structFieldType("struct"),
+      _structArrayFieldType(_structFieldType),
+      _structMapFieldType(mapKeyIsByte ? *DataType::BYTE : *DataType::STRING, _structFieldType),
+      _mapFieldType(mapKeyIsByte ? *DataType::BYTE : *DataType::STRING, *DataType::STRING)
 {
-    DocumenttypesConfigBuilderHelper builder;
-    builder.document(doc_type_id, type_name,
-                     Struct(header_name), Struct(body_name).
-                     addField("f1", DataType::T_STRING).
-                     addField("f2", DataType::T_STRING).
-                     addField("f3", DataType::T_INT).
-                     addField("f5", DataType::T_STRING));
-    return std::unique_ptr<const DocumentTypeRepo>(new DocumentTypeRepo(builder.config()));
+    addFields(_documentType, fieldF3IsString, hasFieldF4, hasFieldF5);
+    if (hasStruct) {
+        addFields(_structFieldType, fieldF3IsString, hasFieldF4, hasFieldF5);
+        _documentType.addField(Field("sarray", 11, _structArrayFieldType, true));
+        _documentType.addField(Field("smap", 12, _structMapFieldType, true));
+        _documentType.addField(Field("map", 13, _mapFieldType, true));
+    }
 }
 
+DocumentTypeFixture::~DocumentTypeFixture() = default;
+
 struct Fixture
 {
-    std::unique_ptr<const DocumentTypeRepo> _oldRepo;
-    std::unique_ptr<const DocumentTypeRepo> _newRepo;
+    DocumentTypeFixture _oldDocType;
+    DocumentTypeFixture _newDocType;
     DocumentTypeInspector _inspector;
-    Fixture()
-        : _oldRepo(makeOldDocTypeRepo()),
-          _newRepo(makeNewDocTypeRepo()),
-          _inspector(*_oldRepo->getDocumentType("test"), *_newRepo->getDocumentType("test"))
+    Fixture(bool hasStruct = true, bool mapKeyIsByte = false)
+        : _oldDocType(true, true, false, hasStruct, mapKeyIsByte),
+          _newDocType(false, false, true, true, false),
+          _inspector(_oldDocType._documentType, _newDocType._documentType)
     {
     }
 };
 
 TEST_F("require that unchanged fields are known", Fixture)
 {
-    EXPECT_TRUE(f._inspector.hasUnchangedField("f1"));
-    EXPECT_TRUE(f._inspector.hasUnchangedField("f2"));
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_TRUE(inspector.hasUnchangedField("f1"));
+    EXPECT_TRUE(inspector.hasUnchangedField("f2"));
+    EXPECT_TRUE(inspector.hasUnchangedField("sarray.f1"));
+    EXPECT_TRUE(inspector.hasUnchangedField("sarray.f2"));
+    EXPECT_TRUE(inspector.hasUnchangedField("smap.key"));
+    EXPECT_TRUE(inspector.hasUnchangedField("smap.value.f1"));
+    EXPECT_TRUE(inspector.hasUnchangedField("smap.value.f2"));
+    EXPECT_TRUE(inspector.hasUnchangedField("map.key"));
+    EXPECT_TRUE(inspector.hasUnchangedField("map.value"));
 }
 
 TEST_F("require that changed fields are detected", Fixture)
 {
-    EXPECT_FALSE(f._inspector.hasUnchangedField("f3"));
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_FALSE(inspector.hasUnchangedField("f3"));
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.f3"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f3"));
 }
 
 TEST_F("require that partially missing fields are detected", Fixture)
 {
-    EXPECT_FALSE(f._inspector.hasUnchangedField("f4"));
-    EXPECT_FALSE(f._inspector.hasUnchangedField("f5"));
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_FALSE(inspector.hasUnchangedField("f4"));
+    EXPECT_FALSE(inspector.hasUnchangedField("f5"));
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.f4"));
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.f5"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f4"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f5"));
 }
 
 TEST_F("require that non-existing fields are NOT known", Fixture)
 {
-    EXPECT_FALSE(f._inspector.hasUnchangedField("not"));
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_FALSE(inspector.hasUnchangedField("not"));
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.not"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.not"));
+}
+
+TEST_F("require that map key type change is detected", Fixture(true, true))
+{
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.key"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f1"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f2"));
+    EXPECT_FALSE(inspector.hasUnchangedField("map.key"));
+    EXPECT_FALSE(inspector.hasUnchangedField("map.value"));
+}
+
+TEST_F("require that struct addition is detected", Fixture(false, false))
+{
+    const IDocumentTypeInspector &inspector = f._inspector;
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.f1"));
+    EXPECT_FALSE(inspector.hasUnchangedField("sarray.f2"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.key"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f1"));
+    EXPECT_FALSE(inspector.hasUnchangedField("smap.value.f2"));
+    EXPECT_FALSE(inspector.hasUnchangedField("map.key"));
+    EXPECT_FALSE(inspector.hasUnchangedField("map.value"));
 }
 
 TEST_MAIN()
diff --git a/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/attribute_reprocessing_initializer_test.cpp b/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/attribute_reprocessing_initializer_test.cpp
index 93c192b57e8..7be774f7291 100644
--- a/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/attribute_reprocessing_initializer_test.cpp
+++ b/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/attribute_reprocessing_initializer_test.cpp
@@ -306,6 +306,18 @@ TEST("require that added attribute aspect with flushed attribute after interrupt
     EXPECT_TRUE(f.assertAttributes({}));
 }
 
+TEST_F("require that removed attribute aspect from struct field does not require document field populate", Fixture)
+{
+    f.addOldConfig({"array.a"}, {"array.a"}).addNewConfig({"array.a"}, {}).init();
+    EXPECT_TRUE(f.assertFields({}));
+}
+
+TEST_F("require that added attribute aspect to struct field requires attribute populate", Fixture)
+{
+    f.addOldConfig({"array.a"}, {}).addNewConfig({"array.a"}, {"array.a"}).init();
+    EXPECT_TRUE(f.assertAttributes({"array.a"}));
+}
+
 TEST_MAIN()
 {
     TEST_RUN_ALL();
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp
index 9807faa5021..cf803ec0368 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp
@@ -29,11 +29,15 @@ bool fastPartialUpdateAttribute(const search::attribute::Config &cfg) {
             (basicType != BasicType::Type::REFERENCE));
 }
 
+bool isStructFieldAttribute(const vespalib::string &name) {
+    return name.find('.') != vespalib::string::npos;
+}
+
 bool willTriggerReprocessOnAttributeAspectRemoval(const search::attribute::Config &cfg,
                                                   const IIndexschemaInspector &indexschemaInspector,
                                                   const vespalib::string &name)
 {
-    return fastPartialUpdateAttribute(cfg) && !indexschemaInspector.isStringIndex(name);
+    return fastPartialUpdateAttribute(cfg) && !indexschemaInspector.isStringIndex(name) && !isStructFieldAttribute(name);
 }
 
 
@@ -73,6 +77,7 @@ handleNewAttributes(const AttributesConfig &oldAttributesConfig,
                     SummarymapConfigBuilder &summarymapConfig)
 {
     vespalib::hash_set<vespalib::string> delayed;
+    vespalib::hash_set<vespalib::string> delayedStruct;
     AttributesConfigHash oldAttrs(oldAttributesConfig.attribute);
     for (const auto &newAttr : newAttributesConfig.attribute) {
         search::attribute::Config newCfg = ConfigConverter::convert(newAttr);
@@ -102,6 +107,10 @@ handleNewAttributes(const AttributesConfig &oldAttributesConfig,
             } else {
                 // Delay addition of attribute aspect
                 delayed.insert(newAttr.name);
+                auto pos = newAttr.name.find('.');
+                if (pos != vespalib::string::npos) {
+                    delayedStruct.insert(newAttr.name.substr(0, pos));
+                }
             }
         }
     }
@@ -111,6 +120,11 @@ handleNewAttributes(const AttributesConfig &oldAttributesConfig,
             if (itr == delayed.end()) {
                 summarymapConfig.override.emplace_back(override);
             }
+        } else if (override.command == "attributecombiner") {
+            auto itr = delayedStruct.find(override.field);
+            if (itr == delayedStruct.end()) {
+                summarymapConfig.override.emplace_back(override);
+            }
         } else {
             summarymapConfig.override.emplace_back(override);
         }
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
index 239f91b449f..bc49b300a1a 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
@@ -47,6 +47,8 @@ AttributeWriter::WriteField::buildFieldPath(const DocumentType &docType)
         docType.buildFieldPath(fp, name);
     } catch (document::FieldNotFoundException & e) {
         fp = FieldPath();
+    } catch (vespalib::IllegalArgumentException &e) {
+        fp = FieldPath();
     }
     _fieldPath = std::move(fp);
 }
diff --git a/searchcore/src/vespa/searchcore/proton/common/document_type_inspector.cpp b/searchcore/src/vespa/searchcore/proton/common/document_type_inspector.cpp
index 6cff162ae08..e19fa5351c2 100644
--- a/searchcore/src/vespa/searchcore/proton/common/document_type_inspector.cpp
+++ b/searchcore/src/vespa/searchcore/proton/common/document_type_inspector.cpp
@@ -1,6 +1,11 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "document_type_inspector.h"
+#include <vespa/document/base/exceptions.h>
+#include <vespa/document/base/fieldpath.h>
+
+using document::FieldPath;
+using document::FieldPathEntry;
 
 namespace proton {
 
@@ -14,12 +19,28 @@ DocumentTypeInspector::DocumentTypeInspector(const document::DocumentType &oldDo
 bool
 DocumentTypeInspector::hasUnchangedField(const vespalib::string &name) const
 {
-    if (!_oldDocType.hasField(name) || !_newDocType.hasField(name)) {
+    FieldPath oldPath;
+    FieldPath newPath;
+    try {
+        _oldDocType.buildFieldPath(oldPath, name);
+        _newDocType.buildFieldPath(newPath, name);
+    } catch (document::FieldNotFoundException &e) {
+        return false;
+    } catch (vespalib::IllegalArgumentException &e) {
         return false;
     }
-    const document::Field &oldField = _oldDocType.getField(name);
-    const document::Field &newField = _newDocType.getField(name);
-    return oldField == newField;
+    if (oldPath.size() != newPath.size()) {
+        return false;
+    }
+    for (uint32_t i = 0; i < oldPath.size(); ++i) {
+        const auto &oldEntry = oldPath[i];
+        const auto &newEntry = newPath[i];
+        if (oldEntry.getType() != newEntry.getType() ||
+            oldEntry.getDataType() != newEntry.getDataType()) {
+            return false;
+        }
+    }
+    return true;
 }
 
 } // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp b/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp
index 711f8ebdcaf..1df812b0c61 100644
--- a/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp
@@ -38,6 +38,9 @@ bool fastPartialUpdateAttribute(BasicType::Type attrType) {
             (attrType != BasicType::Type::REFERENCE));
 }
 
+bool isStructFieldAttribute(const vespalib::string &name) {
+    return name.find('.') != vespalib::string::npos;
+}
 
 FilterAttributeManager::AttributeSet
 getAttributeSetToPopulate(const ARIConfig &newCfg,
@@ -100,7 +103,8 @@ getFieldsToPopulate(const ARIConfig &newCfg,
         // keep the original in order to preserve annotations.
         bool wasStringIndexField = oldIndexschemaInspector.isStringIndex(name);
         bool populateField = !inNewAttrMgr && unchangedField && !wasStringIndexField &&
-                             fastPartialUpdateAttribute(attrType.type());
+                             fastPartialUpdateAttribute(attrType.type()) &&
+                             !isStructFieldAttribute(name);
         LOG(debug, "getFieldsToPopulate(): name='%s', inNewAttrMgr=%s, unchangedField=%s, "
                 "wasStringIndexField=%s, dataType=%s, populate=%s",
                 name.c_str(), toStr(inNewAttrMgr), toStr(unchangedField),