diff options
author | Geir Storli <geirstorli@yahoo.no> | 2018-06-13 15:11:53 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-06-13 15:11:53 +0200 |
commit | 0b3ab662c8c5b7eab2dce927f3aead05750eaecd (patch) | |
tree | 48cf30cf2d59d99cbd1f2d387843893e4448da58 /searchcore | |
parent | 0fefd4e3b3038db6b3b2f573571034d3614816bf (diff) | |
parent | b575c8c294d56b522df73c87ffabed3e49526639 (diff) |
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.
Diffstat (limited to 'searchcore')
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), |