Merge pull request #24160 from vespa-engine/toregge/reduce-special-handling-of-struct-fields

Reduce special handling of struct fields.

5 files changed, 62 insertions, 72 deletions

diff --git a/searchsummary/src/tests/docsummary/slime_filler/slime_filler_test.cpp b/searchsummary/src/tests/docsummary/slime_filler/slime_filler_test.cpp
index 49be0caefc1..fb902eda080 100644
--- a/searchsummary/src/tests/docsummary/slime_filler/slime_filler_test.cpp
+++ b/searchsummary/src/tests/docsummary/slime_filler/slime_filler_test.cpp
@@ -183,9 +183,10 @@ protected:
     void expect_insert_filtered(const vespalib::string& exp, const FieldValue& fv, const std::vector<uint32_t>& matching_elems);
     void expect_insert(const vespalib::string& exp, const FieldValue& fv, SlimeFillerFilter& filter);
     void expect_insert_callback(const std::vector<vespalib::string>& exp, const FieldValue& fv);
-    // Following 3 member functions tests static member functions in SlimeFiller
+    // Following 4 member functions tests static member functions in SlimeFiller
     void expect_insert_summary_field(const vespalib::string& exp, const FieldValue& fv);
     void expect_insert_summary_field_with_filter(const vespalib::string& exp, const FieldValue& fv, const std::vector<uint32_t>& matching_elems);
+    void expect_insert_summary_field_with_field_filter(const vespalib::string& exp, const FieldValue& fv, const SlimeFillerFilter* filter);
     void expect_insert_juniper_field(const std::vector<vespalib::string>& exp, const vespalib::string& exp_slime, const FieldValue& fv);
 };
 
@@ -346,6 +347,16 @@ SlimeFillerTest::expect_insert_summary_field_with_filter(const vespalib::string&
 }
 
 void
+SlimeFillerTest::expect_insert_summary_field_with_field_filter(const vespalib::string& exp, const FieldValue& fv, const SlimeFillerFilter* filter)
+{
+    Slime slime;
+    SlimeInserter inserter(slime);
+    SlimeFiller::insert_summary_field_with_field_filter(fv, inserter, filter);
+    auto act = slime_to_string(slime);
+    EXPECT_EQ(exp, act);
+}
+
+void
 SlimeFillerTest::expect_insert_juniper_field(const std::vector<vespalib::string>& exp, const vespalib::string& exp_slime, const FieldValue& fv)
 {
     Slime slime;
@@ -609,6 +620,16 @@ TEST_F(SlimeFillerTest, insert_summary_field_with_filter)
     expect_insert_summary_field_with_filter("null", make_empty_map(), {});
 }
 
+TEST_F(SlimeFillerTest, insert_summary_field_with_field_filter)
+{
+    auto nested = make_nested_value(0);
+    // Field order depends on assigned field ids, cf. document::Field::calculateIdV7(), and symbol insertion order in slime
+    expect_insert_summary_field_with_field_filter(R"({"f":{"c":66,"a":62},"c":46,"a":42,"b":44,"d":{"c":66,"a":62}})", nested, nullptr);
+    SlimeFillerFilter filter;
+    filter.add("a").add("c").add("f.a").add("d");
+    expect_insert_summary_field_with_field_filter(R"({"f":{"a":62},"a":42,"c":46,"d":{"a":62,"c":66}})", nested, &filter);
+}
+
 TEST_F(SlimeFillerTest, insert_juniper_field)
 {
     expect_insert_juniper_field({"Hello"}, "null", StringFieldValue("Hello"));
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp
index 06cb363b61f..1baa6bca189 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp
+++ b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.cpp
@@ -365,6 +365,17 @@ SlimeFiller::insert_summary_field_with_filter(const FieldValue& value, vespalib:
 }
 
 void
+SlimeFiller::insert_summary_field_with_field_filter(const document::FieldValue& value, vespalib::slime::Inserter& inserter, const SlimeFillerFilter* filter)
+{
+    CheckUndefinedValueVisitor check_undefined;
+    value.accept(check_undefined);
+    if (!check_undefined.is_undefined()) {
+        SlimeFiller visitor(inserter, nullptr, filter);
+        value.accept(visitor);
+    }
+}
+
+void
 SlimeFiller::insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter)
 {
     CheckUndefinedValueVisitor check_undefined;
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h
index 2f924de8af4..620610d782d 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h
+++ b/searchsummary/src/vespa/searchsummary/docsummary/slime_filler.h
@@ -64,6 +64,7 @@ public:
      * Insert the given field value, but only the elements that are contained in the matching_elems vector.
      */
     static void insert_summary_field_with_filter(const document::FieldValue& value, vespalib::slime::Inserter& inserter, const std::vector<uint32_t>& matching_elems);
+    static void insert_summary_field_with_field_filter(const document::FieldValue& value, vespalib::slime::Inserter& inserter, const SlimeFillerFilter* filter);
     static void insert_juniper_field(const document::FieldValue& value, vespalib::slime::Inserter& inserter, IStringFieldConverter& converter);
 };
 
diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp
index 845e81dc01b..398404c6e71 100644
--- a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp
+++ b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.cpp
@@ -2,7 +2,6 @@
 
 #include "docsumfilter.h"
 #include <vespa/juniper/juniper_separators.h>
-#include <vespa/searchsummary/docsummary/check_undefined_value_visitor.h>
 #include <vespa/searchsummary/docsummary/i_docsum_store_document.h>
 #include <vespa/searchsummary/docsummary/i_juniper_converter.h>
 #include <vespa/searchsummary/docsummary/i_string_field_converter.h>
@@ -40,6 +39,19 @@ bool is_struct_or_multivalue_field_type(const FieldPath& fp)
     return false;
 }
 
+
+std::optional<FieldIdT>
+get_single_source_field_id(const DocsumFieldSpec &field_spec)
+{
+    if (field_spec.is_struct_or_multivalue()) {
+        return field_spec.getOutputField().getId();
+    }
+    if (field_spec.getInputFields().size() == 1 && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) {
+        return field_spec.getInputFields()[0].getId();
+    }
+    return std::nullopt; // No single source
+}
+
 FieldPath
 copyPathButFirst(const FieldPath & rhs) {
     // skip the element that correspond to the start field value
@@ -100,34 +112,6 @@ prepareFieldSpec(DocsumFieldSpec & spec, const DocsumTools::FieldSpec & toolsSpe
     }
 }
 
-search::docsummary::DocsumStoreFieldValue
-get_struct_or_multivalue_summary_field(const DocsumFieldSpec& field_spec, const Document& doc)
-{
-    // Filtering not yet implemented, return whole struct or multivalue field
-    const DocsumFieldSpec::FieldIdentifier & fieldId = field_spec.getOutputField();
-    const document::FieldValue* field_value = doc.getField(fieldId.getId());
-    return DocsumStoreFieldValue(field_value);
-}
-
-void
-insert_struct_or_multivalue_summary_field(const DocsumFieldSpec& field_spec, const Document& doc, vespalib::slime::Inserter& inserter)
-{
-    if (field_spec.getCommand() != VsmsummaryConfig::Fieldmap::Command::NONE) {
-        return;
-    }
-    const DocsumFieldSpec::FieldIdentifier& fieldId = field_spec.getOutputField();
-    const document::FieldValue* fv = doc.getField(fieldId.getId());
-    if (fv == nullptr) {
-        return;
-    }
-    CheckUndefinedValueVisitor check_undefined;
-    fv->accept(check_undefined);
-    if (!check_undefined.is_undefined()) {
-        SlimeFiller writer(inserter, nullptr, field_spec.get_filter());
-        fv->accept(writer);
-    }
-}
-
 /*
  * This class creates a modified field value which is then passed to
  * the original juniper converter.
@@ -386,8 +370,14 @@ DocsumFilter::get_document(uint32_t id)
 }
 
 search::docsummary::DocsumStoreFieldValue
-DocsumFilter::get_flattened_summary_field(const DocsumFieldSpec& field_spec, const Document& doc)
+DocsumFilter::get_summary_field(uint32_t entry_idx, const Document& doc)
 {
+    const auto& field_spec = _fields[entry_idx];
+    auto single_source_field_id = get_single_source_field_id(field_spec);
+    if (single_source_field_id.has_value()) {
+        auto field_value = doc.getField(single_source_field_id.value());
+        return DocsumStoreFieldValue(field_value);
+    }
     if (!write_flatten_field(field_spec, doc)) {
         return {};
     }
@@ -397,28 +387,18 @@ DocsumFilter::get_flattened_summary_field(const DocsumFieldSpec& field_spec, con
     return DocsumStoreFieldValue(std::move(value));
 }
 
-search::docsummary::DocsumStoreFieldValue
-DocsumFilter::get_summary_field(uint32_t entry_idx, const Document& doc)
+void
+DocsumFilter::insert_summary_field(uint32_t entry_idx, const Document& doc, vespalib::slime::Inserter& inserter)
 {
     const auto& field_spec = _fields[entry_idx];
-    if (field_spec.is_struct_or_multivalue()) {
-        return get_struct_or_multivalue_summary_field(field_spec, doc);
-    } else {
-        if (field_spec.getInputFields().size() == 1 && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) {
-            const DocsumFieldSpec::FieldIdentifier & fieldId = field_spec.getInputFields()[0];
-            const document::FieldValue* field_value = doc.getField(fieldId.getId());
-            return DocsumStoreFieldValue(field_value);
-        } else if (field_spec.getInputFields().empty() && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) {
-            return {};
-        } else {
-            return get_flattened_summary_field(field_spec, doc);
+    auto single_source_field_id = get_single_source_field_id(field_spec);
+    if (single_source_field_id.has_value()) {
+        auto field_value = doc.getField(single_source_field_id.value());
+        if (field_value != nullptr) {
+            SlimeFiller::insert_summary_field_with_field_filter(*field_value, inserter, field_spec.get_filter());
         }
+        return;
     }
-}
-
-void
-DocsumFilter::insert_flattened_summary_field(const DocsumFieldSpec& field_spec, const Document& doc, vespalib::slime::Inserter& inserter)
-{
     if (!write_flatten_field(field_spec, doc)) {
         return;
     }
@@ -427,26 +407,6 @@ DocsumFilter::insert_flattened_summary_field(const DocsumFieldSpec& field_spec,
     _flattenWriter.clear();
 }
 
-void
-DocsumFilter::insert_summary_field(uint32_t entry_idx, const Document& doc, vespalib::slime::Inserter& inserter)
-{
-    const auto& field_spec = _fields[entry_idx];
-    if (field_spec.is_struct_or_multivalue()) {
-        insert_struct_or_multivalue_summary_field(field_spec, doc, inserter);
-    } else {
-        if (field_spec.getInputFields().size() == 1 && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) {
-            const DocsumFieldSpec::FieldIdentifier & fieldId = field_spec.getInputFields()[0];
-            const document::FieldValue* field_value = doc.getField(fieldId.getId());
-            if (field_value != nullptr) {
-                SlimeFiller::insert_summary_field(*field_value, inserter);
-            }
-        } else if (field_spec.getInputFields().empty() && field_spec.getCommand() == VsmsummaryConfig::Fieldmap::Command::NONE) {
-        } else {
-            insert_flattened_summary_field(field_spec, doc, inserter);
-        }
-    }
-}
-
 FieldModifier*
 DocsumFilter::get_field_modifier(uint32_t entry_idx)
 {
diff --git a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.h b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.h
index d7f6d88bd0c..e87a3e9a431 100644
--- a/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.h
+++ b/streamingvisitors/src/vespa/vsm/vsm/docsumfilter.h
@@ -38,9 +38,6 @@ private:
                                                VsmsummaryConfig::Fieldmap::Command command,
                                                const Document & docsum, bool & modified);
     bool write_flatten_field(const DocsumFieldSpec& field_spec, const Document & docsum);
-
-    search::docsummary::DocsumStoreFieldValue get_flattened_summary_field(const DocsumFieldSpec& field_spec, const Document& doc);
-    void insert_flattened_summary_field(const DocsumFieldSpec& field_spec, const Document& doc, vespalib::slime::Inserter& inserter);
 public:
     DocsumFilter(DocsumToolsPtr tools, const IDocSumCache & docsumCache);
     DocsumFilter(const DocsumFilter &) = delete;