Merge pull request #8715 from vespa-engine/geirst/optimize-updates-on-tensor-attributes

Geirst/optimize updates on tensor attributes

12 files changed, 213 insertions, 162 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 a9e8cbf9675..7141dc7baf7 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
@@ -204,7 +204,7 @@ public:
     void assertAttributeConfig(const std::vector<AttributesConfig::Attribute> &exp)
     {
         auto actConfig = _delayer.getAttributesConfig();
-        EXPECT_TRUE(exp == actConfig->attribute);
+        EXPECT_EQ(exp, actConfig->attribute);
     }
     void assertSummarymapConfig(const std::vector<SummarymapConfig::Override> &exp)
     {
@@ -290,14 +290,24 @@ TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_if_als
     assertSummarymapConfig({});
 }
 
-TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_tensor)
+TEST_F(DelayerTest, require_that_adding_attribute_aspect_is_delayed_for_tensor_field)
 {
     addFields({"a"});
-    setup(attrCfg({make_tensor_cfg("tensor(x[10])")}), smCfg({make_attribute_override("a")}), attrCfg({}), sCfg({make_summary_field("a", "tensor")}), smCfg({}));
+    setup(attrCfg({}), smCfg({}),
+          attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")}));
     assertAttributeConfig({});
     assertSummarymapConfig({});
 }
 
+TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_delayed_for_tensor_field)
+{
+    addFields({"a"});
+    setup(attrCfg({make_tensor_cfg("tensor(x[10])")}), smCfg({make_attribute_override("a")}),
+          attrCfg({}), sCfg({make_summary_field("a", "tensor")}), smCfg({}));
+    assertAttributeConfig({make_tensor_cfg("tensor(x[10])")});
+    assertSummarymapConfig({make_attribute_override("a")});
+}
+
 TEST_F(DelayerTest, require_that_removing_attribute_aspect_is_not_delayed_for_predicate)
 {
     addFields({"a"});
@@ -330,19 +340,21 @@ TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_true_false_e
     assertSummarymapConfig({make_attribute_override("a")});
 }
 
-TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_false_true_edge_on_tensor_attr)
+TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_false_true_edge_on_tensor_attribute)
 {
     addFields({"a"});
-    setup(attrCfg({make_tensor_cfg("tensor(x[10])")}), smCfg({make_attribute_override("a")}), attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")}));
+    setup(attrCfg({make_tensor_cfg("tensor(x[10])")}), smCfg({make_attribute_override("a")}),
+          attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")}));
     assertAttributeConfig({make_tensor_cfg("tensor(x[10])")});
     assertSummarymapConfig({make_attribute_override("a")});
 }
 
-TEST_F(DelayerTest, require_that_fast_access_flag_change_is_not_delayed_true_false_edge_on_tensor_attr)
+TEST_F(DelayerTest, require_that_fast_access_flag_change_is_delayed_true_false_edge_on_tensor_attribute)
 {
     addFields({"a"});
-    setup(attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), smCfg({make_attribute_override("a")}), attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")}));
-    assertAttributeConfig({make_tensor_cfg("tensor(x[10])")});
+    setup(attrCfg({make_fa(make_tensor_cfg("tensor(x[10])"))}), smCfg({make_attribute_override("a")}),
+          attrCfg({make_tensor_cfg("tensor(x[10])")}), sCfg({make_summary_field("a", "tensor")}), smCfg({make_attribute_override("a")}));
+    assertAttributeConfig({make_fa(make_tensor_cfg("tensor(x[10])"))});
     assertSummarymapConfig({make_attribute_override("a")});
 }
 
diff --git a/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp b/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
index 0f9df1fc594..fd1984a79fe 100644
--- a/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/feedview/feedview_test.cpp
@@ -1070,46 +1070,43 @@ void putDocumentAndUpdate(Fixture &f, const vespalib::string &fieldName)
 }
 
 template <typename Fixture>
-void requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(Fixture &f)
+void requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(Fixture &f,
+                                                              const vespalib::string &fieldName)
 {
-    putDocumentAndUpdate(f, "a1");
+    putDocumentAndUpdate(f, fieldName);
 
     EXPECT_EQUAL(1u, f.msa._store._lastSyncToken); // document store not updated
     assertAttributeUpdate(2u, DocumentId("doc:test:1"), 1, f.maw);
 }
 
+template <typename Fixture>
+void requireThatUpdateUpdatesAttributeAndDocumentStore(Fixture &f,
+                                                       const vespalib::string &fieldName)
+{
+    putDocumentAndUpdate(f, fieldName);
+
+    EXPECT_EQUAL(2u, f.msa._store._lastSyncToken); // document store updated
+    assertAttributeUpdate(2u, DocumentId("doc:test:1"), 1, f.maw);
+}
+
 TEST_F("require that update() to fast-access attribute only updates attribute and not document store",
        FastAccessFeedViewFixture)
 {
     f.maw._attrs.insert("a1"); // mark a1 as fast-access attribute field
-    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f);
+    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f, "a1");
 }
 
 TEST_F("require that update() to attribute only updates attribute and not document store",
        SearchableFeedViewFixture)
 {
     f.maw._attrs.insert("a1"); // mark a1 as attribute field
-    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f);
+    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f, "a1");
 }
 
 TEST_F("require that update to non fast-access attribute also updates document store",
         FastAccessFeedViewFixture)
 {
-    putDocumentAndUpdate(f, "a1");
-
-    EXPECT_EQUAL(2u, f.msa._store._lastSyncToken); // document store updated
-    assertAttributeUpdate(2u, DocumentId("doc:test:1"), 1, f.maw);
-}
-
-template <typename Fixture>
-void requireThatUpdateUpdatesAttributeAndDocumentStore(Fixture &f,
-                                                       const vespalib::string &
-                                                       fieldName)
-{
-    putDocumentAndUpdate(f, fieldName);
-
-    EXPECT_EQUAL(2u, f.msa._store._lastSyncToken); // document store updated
-    assertAttributeUpdate(2u, DocumentId("doc:test:1"), 1, f.maw);
+    requireThatUpdateUpdatesAttributeAndDocumentStore(f, "a1");
 }
 
 TEST_F("require that update() to fast-access predicate attribute updates attribute and document store",
@@ -1126,18 +1123,18 @@ TEST_F("require that update() to predicate attribute updates attribute and docum
     requireThatUpdateUpdatesAttributeAndDocumentStore(f, "a2");
 }
 
-TEST_F("require that update() to fast-access tensor attribute updates attribute and document store",
+TEST_F("require that update() to fast-access tensor attribute only updates attribute and NOT document store",
        FastAccessFeedViewFixture)
 {
     f.maw._attrs.insert("a3"); // mark a3 as fast-access attribute field
-    requireThatUpdateUpdatesAttributeAndDocumentStore(f, "a3");
+    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f, "a3");
 }
 
-TEST_F("require that update() to tensor attribute updates attribute and document store",
+TEST_F("require that update() to tensor attribute only updates attribute and NOT document store",
        SearchableFeedViewFixture)
 {
     f.maw._attrs.insert("a3"); // mark a3 as attribute field
-    requireThatUpdateUpdatesAttributeAndDocumentStore(f, "a3");
+    requireThatUpdateOnlyUpdatesAttributeAndNotDocumentStore(f, "a3");
 }
 
 TEST_F("require that compactLidSpace() propagates to document meta store and document store and "
diff --git a/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/CMakeLists.txt b/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/CMakeLists.txt
index d3abd6757e2..048ed1938e5 100644
--- a/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/CMakeLists.txt
+++ b/searchcore/src/tests/proton/reprocessing/attribute_reprocessing_initializer/CMakeLists.txt
@@ -6,5 +6,6 @@ vespa_add_executable(searchcore_attribute_reprocessing_initializer_test_app TEST
     searchcore_reprocessing
     searchcore_attribute
     searchcore_pcommon
+    gtest
 )
 vespa_add_test(NAME searchcore_attribute_reprocessing_initializer_test_app COMMAND searchcore_attribute_reprocessing_initializer_test_app)
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 219c2c42bd7..95cbdafb8e4 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
@@ -14,8 +14,8 @@
 #include <vespa/searchlib/common/foregroundtaskexecutor.h>
 #include <vespa/searchlib/index/dummyfileheadercontext.h>
 #include <vespa/searchlib/test/directory_handler.h>
+#include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/test/insertion_operators.h>
-#include <vespa/vespalib/testkit/testapp.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP("attribute_reprocessing_initializer_test");
@@ -126,8 +126,8 @@ struct MyIndexschemaInspector : public IIndexschemaInspector
     }
 };
 
-struct Fixture
-{
+class InitializerTest : public ::testing::Test {
+public:
     DirectoryHandler _dirHandler;
     DummyFileHeaderContext _fileHeaderContext;
     ForegroundTaskExecutor _attributeFieldWriter;
@@ -138,7 +138,7 @@ struct Fixture
     MyDocTypeInspector _inspector;
     AttributeReprocessingInitializer::UP _initializer;
     MyReprocessingHandler _handler;
-    Fixture()
+    InitializerTest()
         : _dirHandler(TEST_DIR),
           _fileHeaderContext(),
           _attributeFieldWriter(),
@@ -153,7 +153,7 @@ struct Fixture
           _handler()
     {
     }
-    ~Fixture() { }
+    ~InitializerTest() { }
     void init() {
         MyIndexschemaInspector oldIndexschemaInspector(_oldCfg._schema);
         _initializer.reset(new AttributeReprocessingInitializer
@@ -163,20 +163,20 @@ struct Fixture
                             "test", INIT_SERIAL_NUM));
         _initializer->initialize(_handler);
     }
-    Fixture &addOldConfig(const StringVector &fields, const StringVector &attrs) {
+    InitializerTest &addOldConfig(const StringVector &fields, const StringVector &attrs) {
         return addConfig(fields, attrs, _oldCfg);
     }
-    Fixture &addNewConfig(const StringVector &fields, const StringVector &attrs) {
+    InitializerTest &addNewConfig(const StringVector &fields, const StringVector &attrs) {
         return addConfig(fields, attrs, _newCfg);
     }
-    Fixture &addConfig(const StringVector &fields, const StringVector &attrs, MyConfig &cfg) {
+    InitializerTest &addConfig(const StringVector &fields, const StringVector &attrs, MyConfig &cfg) {
         cfg.addFields(fields);
         cfg.addAttrs(attrs);
         return *this;
     }
-    bool assertAttributes(const StringSet &expAttrs) {
+    void assertAttributes(const StringSet &expAttrs) {
         if (expAttrs.empty()) {
-            if (!EXPECT_TRUE(_handler._reader.get() == nullptr)) return false;
+            EXPECT_TRUE(_handler._reader.get() == nullptr);;
         } else {
             const auto & populator = dynamic_cast<const AttributePopulator &>(*_handler._reader);
             std::vector<search::AttributeVector *> attrList = populator.getWriter().getWritableAttributes();
@@ -184,102 +184,112 @@ struct Fixture
             for (const auto attr : attrList) {
                 actAttrs.insert(attr->getName());
             }
-            if (!EXPECT_EQUAL(expAttrs, actAttrs)) return false;
+            EXPECT_EQ(expAttrs, actAttrs);
         }
-        return true;
     }
-    bool assertFields(const StringSet &expFields) {
+    void assertFields(const StringSet &expFields) {
         if (expFields.empty()) {
-            if (!EXPECT_EQUAL(0u, _handler._rewriters.size())) return false;
+            EXPECT_EQ(0u, _handler._rewriters.size());
         } else {
             StringSet actFields;
             for (auto rewriter : _handler._rewriters) {
                 const auto & populator = dynamic_cast<const DocumentFieldPopulator &>(*rewriter);
                 actFields.insert(populator.getAttribute().getName());
             }
-            if (!EXPECT_EQUAL(expFields, actFields)) return false;
+            EXPECT_EQ(expFields, actFields);
         }
-        return true;
     }
 };
 
-TEST_F("require that new field does NOT require attribute populate", Fixture)
+class Fixture : public InitializerTest {
+    virtual void TestBody() override {}
+};
+
+TEST_F(InitializerTest, require_that_new_field_does_NOT_require_attribute_populate)
 {
-    f.addOldConfig({}, {}).addNewConfig({"a"}, {"a"}).init();
-    EXPECT_TRUE(f.assertAttributes({}));
+    addOldConfig({}, {}).addNewConfig({"a"}, {"a"}).init();
+    assertAttributes({});
 }
 
-TEST_F("require that added attribute aspect does require attribute populate", Fixture)
+TEST_F(InitializerTest, require_that_added_attribute_aspect_does_require_attribute_populate)
 {
-    f.addOldConfig({"a"}, {}).addNewConfig({"a"}, {"a"}).init();
-    EXPECT_TRUE(f.assertAttributes({"a"}));
+    addOldConfig({"a"}, {}).addNewConfig({"a"}, {"a"}).init();
+    assertAttributes({"a"});
 }
 
-TEST_F("require that initializer can setup populate of several attributes", Fixture)
+TEST_F(InitializerTest, require_that_initializer_can_setup_populate_of_several_attributes)
 {
-    f.addOldConfig({"a", "b", "c", "d"}, {"a", "b"}).
+    addOldConfig({"a", "b", "c", "d"}, {"a", "b"}).
             addNewConfig({"a", "b", "c", "d"}, {"a", "b", "c", "d"}).init();
-    EXPECT_TRUE(f.assertAttributes({"c", "d"}));
+    assertAttributes({"c", "d"});
 }
 
-TEST_F("require that new field does NOT require document field populate", Fixture)
+TEST_F(InitializerTest, require_that_new_field_does_NOT_require_document_field_populate)
 {
-    f.addOldConfig({}, {}).addNewConfig({"a"}, {"a"}).init();
-    EXPECT_TRUE(f.assertFields({}));
+    addOldConfig({}, {}).addNewConfig({"a"}, {"a"}).init();
+    assertFields({});
 }
 
-TEST_F("require that removed field does NOT require document field populate", Fixture)
+TEST_F(InitializerTest, require_that_removed_field_does_NOT_require_document_field_populate)
 {
-    f.addOldConfig({"a"}, {"a"}).addNewConfig({}, {}).init();
-    EXPECT_TRUE(f.assertFields({}));
+    addOldConfig({"a"}, {"a"}).addNewConfig({}, {}).init();
+    assertFields({});
 }
 
-TEST_F("require that removed attribute aspect does require document field populate", Fixture)
+TEST_F(InitializerTest, require_that_removed_attribute_aspect_does_require_document_field_populate)
 {
-    f.addOldConfig({"a"}, {"a"}).addNewConfig({"a"}, {}).init();
-    EXPECT_TRUE(f.assertFields({"a"}));
+    addOldConfig({"a"}, {"a"}).addNewConfig({"a"}, {}).init();
+    assertFields({"a"});
 }
 
-TEST_F("require that removed attribute aspect (when also index field) does NOT require document field populate",
-        Fixture)
+TEST_F(InitializerTest, require_that_removed_attribute_aspect_when_also_index_field_does_NOT_require_document_field_populate)
 {
-    f.addOldConfig({"a"}, {"a"}).addNewConfig({"a"}, {});
-    f._oldCfg.addIndexField("a");
-    f._newCfg.addIndexField("a");
-    f.init();
-    EXPECT_TRUE(f.assertFields({}));
+    addOldConfig({"a"}, {"a"}).addNewConfig({"a"}, {});
+    _oldCfg.addIndexField("a");
+    _newCfg.addIndexField("a");
+    init();
+    assertFields({});
 }
 
-TEST_F("require that initializer can setup populate of several document fields", Fixture)
+TEST_F(InitializerTest, require_that_initializer_can_setup_populate_of_several_document_fields)
 {
-    f.addOldConfig({"a", "b", "c", "d"}, {"a", "b", "c", "d"}).
+    addOldConfig({"a", "b", "c", "d"}, {"a", "b", "c", "d"}).
             addNewConfig({"a", "b", "c", "d"}, {"a", "b"}).init();
-    EXPECT_TRUE(f.assertFields({"c", "d"}));
+    assertFields({"c", "d"});
 }
 
-TEST_F("require that initializer can setup both attribute and document field populate", Fixture)
+TEST_F(InitializerTest, require_that_initializer_can_setup_both_attribute_and_document_field_populate)
 {
-    f.addOldConfig({"a", "b"}, {"a"}).
+    addOldConfig({"a", "b"}, {"a"}).
             addNewConfig({"a", "b"}, {"b"}).init();
-    EXPECT_TRUE(f.assertAttributes({"b"}));
-    EXPECT_TRUE(f.assertFields({"a"}));
+    assertAttributes({"b"});
+    assertFields({"a"});
 }
 
-TEST_F("require that tensor fields are not populated from attribute", Fixture)
+TEST_F(InitializerTest, require_that_adding_attribute_aspect_on_tensor_field_require_attribute_populate)
 {
-    f.addOldConfig({"a", "b", "c", "d", "tensor"}, {"a", "b", "c", "d", "tensor"}).
-        addNewConfig({"a", "b", "c", "d", "tensor"}, {"a", "b"}).init();
-    EXPECT_TRUE(f.assertFields({"c", "d"}));
+    addOldConfig({"tensor"}, {}).
+            addNewConfig({"tensor"}, {"tensor"}).init();
+    assertAttributes({"tensor"});
+    assertFields({});
 }
 
-TEST_F("require that predicate fields are not populated from attribute", Fixture)
+TEST_F(InitializerTest, require_that_removing_attribute_aspect_from_tensor_field_require_document_field_populate)
 {
-    f.addOldConfig({"a", "b", "c", "d", "predicate"}, {"a", "b", "c", "d", "predicate"}).
-        addNewConfig({"a", "b", "c", "d", "predicate"}, {"a", "b"}).init();
-    EXPECT_TRUE(f.assertFields({"c", "d"}));
+    addOldConfig({"tensor"}, {"tensor"}).
+            addNewConfig({"tensor"}, {}).init();
+    assertAttributes({});
+    assertFields({"tensor"});
 }
 
-TEST("require that added attribute aspect with flushed attribute after interruptted reprocessing does not require attribute populate")
+TEST_F(InitializerTest, require_that_predicate_fields_are_not_populated_from_attribute)
+{
+    addOldConfig({"a", "b", "c", "d", "predicate"}, {"a", "b", "c", "d", "predicate"}).
+            addNewConfig({"a", "b", "c", "d", "predicate"}, {"a", "b"}).init();
+    assertFields({"c", "d"});
+}
+
+TEST(InterruptedTest, require_that_added_attribute_aspect_with_flushed_attribute_after_interruptted_reprocessing_does_not_require_attribute_populate)
 {
     {
         auto diskLayout = AttributeDiskLayout::create(TEST_DIR);
@@ -295,22 +305,19 @@ TEST("require that added attribute aspect with flushed attribute after interrupt
     }
     Fixture f;
     f.addOldConfig({"a"}, {}).addNewConfig({"a"}, {"a"}).init();
-    EXPECT_TRUE(f.assertAttributes({}));
+    f.assertAttributes({});
 }
 
-TEST_F("require that removed attribute aspect from struct field does not require document field populate", Fixture)
+TEST_F(InitializerTest, require_that_removed_attribute_aspect_from_struct_field_does_not_require_document_field_populate)
 {
-    f.addOldConfig({"array.a"}, {"array.a"}).addNewConfig({"array.a"}, {}).init();
-    EXPECT_TRUE(f.assertFields({}));
+    addOldConfig({"array.a"}, {"array.a"}).addNewConfig({"array.a"}, {}).init();
+    assertFields({});
 }
 
-TEST_F("require that added attribute aspect to struct field requires attribute populate", Fixture)
+TEST_F(InitializerTest, require_that_added_attribute_aspect_to_struct_field_requires_attribute_populate)
 {
-    f.addOldConfig({"array.a"}, {}).addNewConfig({"array.a"}, {"array.a"}).init();
-    EXPECT_TRUE(f.assertAttributes({"array.a"}));
+    addOldConfig({"array.a"}, {}).addNewConfig({"array.a"}, {"array.a"}).init();
+    assertAttributes({"array.a"});
 }
 
-TEST_MAIN()
-{
-    TEST_RUN_ALL();
-}
+GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/CMakeLists.txt b/searchcore/src/vespa/searchcore/proton/attribute/CMakeLists.txt
index 30f6c2d92c2..f7a6ffe7189 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/CMakeLists.txt
+++ b/searchcore/src/vespa/searchcore/proton/attribute/CMakeLists.txt
@@ -17,6 +17,7 @@ vespa_add_library(searchcore_attribute STATIC
     attribute_usage_sampler_context.cpp
     attribute_usage_sampler_functor.cpp
     attribute_usage_stats.cpp
+    attribute_utils.cpp
     attribute_vector_explorer.cpp
     attribute_writer.cpp
     attributes_initializer_base.cpp
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 4cf2df97fd0..4a3f99d5e8e 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_aspect_delayer.cpp
@@ -1,22 +1,24 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "attribute_aspect_delayer.h"
-#include <vespa/searchlib/attribute/configconverter.h>
-#include <vespa/searchcore/proton/common/i_document_type_inspector.h>
-#include <vespa/searchcore/proton/common/i_indexschema_inspector.h>
-#include <vespa/searchcore/proton/common/config_hash.hpp>
-#include <vespa/vespalib/stllike/hash_set.hpp>
 #include <vespa/config-attributes.h>
 #include <vespa/config-summary.h>
 #include <vespa/config-summarymap.h>
+#include <vespa/searchcore/proton/attribute/attribute_utils.h>
+#include <vespa/searchcore/proton/common/config_hash.hpp>
+#include <vespa/searchcore/proton/common/i_document_type_inspector.h>
+#include <vespa/searchcore/proton/common/i_indexschema_inspector.h>
+#include <vespa/searchlib/attribute/configconverter.h>
+#include <vespa/vespalib/stllike/hash_set.hpp>
 
+using proton::attribute::isUpdateableInMemoryOnly;
+using search::attribute::BasicType;
 using search::attribute::ConfigConverter;
 using vespa::config::search::AttributesConfig;
 using vespa::config::search::AttributesConfigBuilder;
+using vespa::config::search::SummaryConfig;
 using vespa::config::search::SummarymapConfig;
 using vespa::config::search::SummarymapConfigBuilder;
-using vespa::config::search::SummaryConfig;
-using search::attribute::BasicType;
 
 namespace proton {
 
@@ -24,22 +26,12 @@ namespace {
 
 using AttributesConfigHash = ConfigHash<AttributesConfig::Attribute>;
 
-bool fastPartialUpdateAttribute(const search::attribute::Config &cfg) {
-    auto basicType = cfg.basicType().type();
-    return ((basicType != BasicType::Type::PREDICATE) &&
-            (basicType != BasicType::Type::TENSOR) &&
-            (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) && !isStructFieldAttribute(name);
+    return isUpdateableInMemoryOnly(name, cfg) &&
+            !indexschemaInspector.isStringIndex(name);
 }
 
 class KnownSummaryFields
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.cpp
new file mode 100644
index 00000000000..bb4ee1da781
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.cpp
@@ -0,0 +1,24 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "attribute_utils.h"
+#include <vespa/searchcommon/attribute/config.h>
+
+namespace proton::attribute {
+
+bool
+isUpdateableInMemoryOnly(const vespalib::string &attrName,
+                         const search::attribute::Config &cfg)
+{
+    auto basicType = cfg.basicType().type();
+    return ((basicType != search::attribute::BasicType::Type::PREDICATE) &&
+            (basicType != search::attribute::BasicType::Type::REFERENCE)) &&
+            !isStructFieldAttribute(attrName);
+}
+
+bool
+isStructFieldAttribute(const vespalib::string &attrName)
+{
+    return attrName.find('.') != vespalib::string::npos;
+}
+
+}
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.h b/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.h
new file mode 100644
index 00000000000..49f6aba8775
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_utils.h
@@ -0,0 +1,31 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+#include <vespa/vespalib/stllike/string.h>
+
+namespace search::attribute { class Config; }
+
+namespace proton::attribute {
+
+/**
+ * Returns whether the given attribute vector is updateable only in-memory.
+ *
+ * For most attributes this is true.
+ * The data stored in the attribute is equal to the data stored in the field value in the document.
+ *
+ * For predicate and reference attributes this is false.
+ * The original data is transformed (lossy) before it is stored in the attribute.
+ * During update we also need to update the field value in the document.
+ *
+ * For struct field attributes this is false.
+ * A struct field attribute typically represents a sub-field of a more complex field (e.g. map of struct or array of struct).
+ * During update the complex field is first updated in the document,
+ * then the struct field attribute is updated based on the new content of the complex field.
+ */
+bool isUpdateableInMemoryOnly(const vespalib::string &attrName,
+                              const search::attribute::Config &cfg);
+
+bool isStructFieldAttribute(const vespalib::string &attrName);
+
+}
diff --git a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
index 9aa2921adf5..34e9ba2c145 100644
--- a/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/attribute/attribute_writer.cpp
@@ -1,18 +1,19 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "attribute_writer.h"
-#include "ifieldupdatecallback.h"
 #include "attributemanager.h"
 #include "document_field_extractor.h"
+#include "ifieldupdatecallback.h"
+#include <vespa/document/base/exceptions.h>
+#include <vespa/document/datatype/documenttype.h>
+#include <vespa/document/fieldvalue/document.h>
+#include <vespa/searchcore/proton/attribute/attribute_utils.h>
 #include <vespa/searchcore/proton/attribute/imported_attributes_repo.h>
 #include <vespa/searchcore/proton/common/attribute_updater.h>
 #include <vespa/searchlib/attribute/attributevector.hpp>
 #include <vespa/searchlib/attribute/imported_attribute_vector.h>
-#include <vespa/searchlib/common/isequencedtaskexecutor.h>
 #include <vespa/searchlib/common/idestructorcallback.h>
-#include <vespa/document/base/exceptions.h>
-#include <vespa/document/datatype/documenttype.h>
-#include <vespa/document/fieldvalue/document.h>
+#include <vespa/searchlib/common/isequencedtaskexecutor.h>
 #include <vespa/vespalib/stllike/hash_map.hpp>
 
 #include <vespa/log/log.h>
@@ -33,7 +34,7 @@ AttributeWriter::WriteField::WriteField(AttributeVector &attribute)
       _structFieldAttribute(false)
 {
     const vespalib::string &name = attribute.getName();
-    _structFieldAttribute = name.find('.') != vespalib::string::npos;
+    _structFieldAttribute = attribute::isStructFieldAttribute(name);
 }
 
 AttributeWriter::WriteField::~WriteField() = default;
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 afae967c4fb..9b1c66780cd 100644
--- a/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.cpp
@@ -2,6 +2,7 @@
 
 #include "attribute_reprocessing_initializer.h"
 #include <vespa/searchcore/proton/attribute/attribute_populator.h>
+#include <vespa/searchcore/proton/attribute/attribute_utils.h>
 #include <vespa/searchcore/proton/attribute/document_field_populator.h>
 #include <vespa/searchcore/proton/attribute/filter_attribute_manager.h>
 #include <vespa/searchcore/proton/common/i_indexschema_inspector.h>
@@ -11,11 +12,12 @@
 LOG_SETUP(".proton.reprocessing.attribute_reprocessing_initializer");
 
 using namespace search::index;
+using proton::attribute::isUpdateableInMemoryOnly;
 using search::AttributeGuard;
 using search::AttributeVector;
 using search::SerialNum;
-using search::index::schema::DataType;
 using search::attribute::BasicType;
+using search::index::schema::DataType;
 
 namespace proton {
 
@@ -31,17 +33,6 @@ toStr(bool value)
     return (value ? "true" : "false");
 }
 
-bool fastPartialUpdateAttribute(BasicType::Type attrType) {
-    // Partial update to tensor or predicate attribute must update document
-    return ((attrType != BasicType::Type::PREDICATE) &&
-            (attrType != BasicType::Type::TENSOR) &&
-            (attrType != BasicType::Type::REFERENCE));
-}
-
-bool isStructFieldAttribute(const vespalib::string &name) {
-    return name.find('.') != vespalib::string::npos;
-}
-
 FilterAttributeManager::AttributeSet
 getAttributeSetToPopulate(const ARIConfig &newCfg,
                           const ARIConfig &oldCfg,
@@ -95,20 +86,19 @@ getFieldsToPopulate(const ARIConfig &newCfg,
     oldCfg.getAttrMgr()->getAttributeList(attrList);
     for (const auto &guard : attrList) {
         const vespalib::string &name = guard->getName();
-        BasicType attrType(guard->getConfig().basicType());
+        const auto &attrCfg = guard->getConfig();
         bool inNewAttrMgr = newCfg.getAttrMgr()->getAttribute(name)->valid();
         bool unchangedField = inspector.hasUnchangedField(name);
         // NOTE: If it is a string and index field we shall
         // keep the original in order to preserve annotations.
         bool wasStringIndexField = oldIndexschemaInspector.isStringIndex(name);
         bool populateField = !inNewAttrMgr && unchangedField && !wasStringIndexField &&
-                             fastPartialUpdateAttribute(attrType.type()) &&
-                             !isStructFieldAttribute(name);
+                             isUpdateableInMemoryOnly(name, attrCfg);
         LOG(debug, "getFieldsToPopulate(): name='%s', inNewAttrMgr=%s, unchangedField=%s, "
                 "wasStringIndexField=%s, dataType=%s, populate=%s",
                 name.c_str(), toStr(inNewAttrMgr), toStr(unchangedField),
             toStr(wasStringIndexField),
-            attrType.asString(),
+            attrCfg.basicType().asString(),
             toStr(populateField));
         if (populateField) {
             fieldsToPopulate.push_back(IReprocessingRewriter::SP
diff --git a/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.h b/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.h
index 8c0b4de4cbc..34302da236e 100644
--- a/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.h
+++ b/searchcore/src/vespa/searchcore/proton/reprocessing/attribute_reprocessing_initializer.h
@@ -15,6 +15,12 @@ class IIndexschemaInspector;
 
 /**
  * Class responsible for initialize reprocessing of attribute vectors if needed.
+ *
+ * 1) Attribute aspect is added to an existing field:
+ *    The attribute is populated based on the content of the field in the document store.
+ *
+ * 2) Attribute aspect is removed from an existing field:
+ *    The field in the document store is populated based on the content of the attribute.
  */
 class AttributeReprocessingInitializer : public IReprocessingInitializer
 {
diff --git a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
index 59b9a655d2a..dbac71138e3 100644
--- a/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/storeonlyfeedview.cpp
@@ -4,21 +4,21 @@
 #include "ireplayconfig.h"
 #include "operationdonecontext.h"
 #include "putdonecontext.h"
+#include "remove_batch_done_context.h"
 #include "removedonecontext.h"
 #include "storeonlyfeedview.h"
 #include "updatedonecontext.h"
-#include "remove_batch_done_context.h"
+#include <vespa/document/datatype/documenttype.h>
+#include <vespa/document/fieldvalue/document.h>
+#include <vespa/document/repo/documenttyperepo.h>
+#include <vespa/searchcore/proton/attribute/attribute_utils.h>
+#include <vespa/searchcore/proton/attribute/ifieldupdatecallback.h>
 #include <vespa/searchcore/proton/common/commit_time_tracker.h>
 #include <vespa/searchcore/proton/common/feedtoken.h>
 #include <vespa/searchcore/proton/documentmetastore/ilidreusedelayer.h>
-#include <vespa/searchcore/proton/reference/i_gid_to_lid_change_handler.h>
-#include <vespa/searchcore/proton/attribute/ifieldupdatecallback.h>
 #include <vespa/searchcore/proton/feedoperation/operations.h>
-
+#include <vespa/searchcore/proton/reference/i_gid_to_lid_change_handler.h>
 #include <vespa/searchlib/common/isequencedtaskexecutor.h>
-#include <vespa/document/datatype/documenttype.h>
-#include <vespa/document/repo/documenttyperepo.h>
-#include <vespa/document/fieldvalue/document.h>
 #include <vespa/vespalib/util/exceptions.h>
 
 #include <vespa/log/log.h>
@@ -29,13 +29,14 @@ using document::Document;
 using document::DocumentId;
 using document::DocumentTypeRepo;
 using document::DocumentUpdate;
-using search::index::Schema;
-using vespalib::makeLambdaTask;
+using proton::attribute::isUpdateableInMemoryOnly;
 using search::IDestructorCallback;
 using search::SerialNum;
+using search::index::Schema;
 using storage::spi::BucketInfoResult;
 using storage::spi::Timestamp;
 using vespalib::IllegalStateException;
+using vespalib::makeLambdaTask;
 using vespalib::make_string;
 
 namespace proton {
@@ -397,21 +398,9 @@ StoreOnlyFeedView::UpdateScope::UpdateScope(const search::index::Schema & schema
       _nonAttributeFields(!upd.getFieldPathUpdates().empty())
 {}
 
-namespace {
-
-bool isAttributeUpdateable(const search::AttributeVector *attribute) {
-    search::attribute::BasicType::Type attrType = attribute->getBasicType();
-    // Partial update to tensor, predicate or reference attribute
-    // must update document
-    return ((attrType != search::attribute::BasicType::Type::PREDICATE) &&
-            (attrType != search::attribute::BasicType::Type::TENSOR) &&
-            (attrType != search::attribute::BasicType::Type::REFERENCE));
-}
-}
-
 void
 StoreOnlyFeedView::UpdateScope::onUpdateField(vespalib::stringref fieldName, const search::AttributeVector * attr) {
-    if (!_nonAttributeFields && (attr == nullptr || !isAttributeUpdateable(attr))) {
+    if (!_nonAttributeFields && (attr == nullptr || !isUpdateableInMemoryOnly(attr->getName(), attr->getConfig()))) {
         _nonAttributeFields = true;
     }
     if (!_indexedFields && _schema->isIndexField(fieldName)) {