Merge pull request #28831 from vespa-engine/bratseth/inherit-multiple-summaries

Bratseth/inherit multiple summaries

10 files changed, 151 insertions, 66 deletions

diff --git a/config-model/src/main/java/com/yahoo/schema/parser/ConvertParsedSchemas.java b/config-model/src/main/java/com/yahoo/schema/parser/ConvertParsedSchemas.java
index 40ec84ec8bc..39a1d21965f 100644
--- a/config-model/src/main/java/com/yahoo/schema/parser/ConvertParsedSchemas.java
+++ b/config-model/src/main/java/com/yahoo/schema/parser/ConvertParsedSchemas.java
@@ -139,12 +139,7 @@ public class ConvertParsedSchemas {
 
     private void convertDocumentSummary(Schema schema, ParsedDocumentSummary parsed, TypeResolver typeContext) {
         var docsum = new DocumentSummary(parsed.name(), schema);
-        var inheritList = parsed.getInherited();
-        if (inheritList.size() == 1) {
-            docsum.setInherited(inheritList.get(0));
-        } else if (inheritList.size() != 0) {
-            throw new IllegalArgumentException("document-summary "+parsed.name()+" cannot inherit more than once");
-        }
+        parsed.getInherited().forEach(inherited -> docsum.addInherited(inherited));
         if (parsed.getFromDisk()) {
             docsum.setFromDisk(true);
         }
diff --git a/config-model/src/main/java/com/yahoo/schema/parser/ParsedDocumentSummary.java b/config-model/src/main/java/com/yahoo/schema/parser/ParsedDocumentSummary.java
index 7aaabaef865..d6648f609cd 100644
--- a/config-model/src/main/java/com/yahoo/schema/parser/ParsedDocumentSummary.java
+++ b/config-model/src/main/java/com/yahoo/schema/parser/ParsedDocumentSummary.java
@@ -10,8 +10,9 @@ import java.util.Map;
  * This class holds the extracted information after parsing a
  * "document-summary" block, using simple data structures as far as
  * possible.  Do not put advanced logic here!
+ *
  * @author arnej27959
- **/
+ */
 class ParsedDocumentSummary extends ParsedBlock {
 
     private boolean omitSummaryFeatures;
@@ -45,4 +46,5 @@ class ParsedDocumentSummary extends ParsedBlock {
     void inherit(String other) {
         inherited.add(other);
     }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/schema/processing/ImplicitSummaryFields.java b/config-model/src/main/java/com/yahoo/schema/processing/ImplicitSummaryFields.java
index b17efbfe8e8..adc7a88cc79 100644
--- a/config-model/src/main/java/com/yahoo/schema/processing/ImplicitSummaryFields.java
+++ b/config-model/src/main/java/com/yahoo/schema/processing/ImplicitSummaryFields.java
@@ -23,7 +23,7 @@ public class ImplicitSummaryFields extends Processor {
     @Override
     public void process(boolean validate, boolean documentsOnly) {
         for (DocumentSummary docsum : schema.getSummariesInThis().values()) {
-            if (docsum.inherited().isPresent()) continue; // Implicit fields are added to inheriting summaries through their parent
+            if ( ! docsum.inherited().isEmpty()) continue; // Implicit fields are added to inheriting summaries through their parent
             addField(docsum, new SummaryField("rankfeatures", DataType.STRING, SummaryTransform.RANKFEATURES), validate);
             addField(docsum, new SummaryField("summaryfeatures", DataType.STRING, SummaryTransform.SUMMARYFEATURES), validate);
         }
diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
index 337f5e11329..359462c3715 100644
--- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
+++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
@@ -2,9 +2,13 @@
 package com.yahoo.vespa.documentmodel;
 
 import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.schema.RankProfile;
 import com.yahoo.schema.Schema;
+import com.yahoo.searchlib.rankingexpression.Reference;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -19,10 +23,9 @@ import java.util.logging.Level;
  */
 public class DocumentSummary extends FieldView {
 
-    private int id;
     private boolean fromDisk = false;
     private boolean omitSummaryFeatures = false;
-    private Optional<String> inherited = Optional.empty();
+    private List<String> inherited = new ArrayList<>();
 
     private final Schema owner;
 
@@ -32,8 +35,6 @@ public class DocumentSummary extends FieldView {
         this.owner = owner;
     }
 
-    public int id() { return id; }
-
     public void setFromDisk(boolean fromDisk) { this.fromDisk = fromDisk; }
 
     /** Returns whether the user has noted explicitly that this summary accesses disk */
@@ -63,15 +64,23 @@ public class DocumentSummary extends FieldView {
         var field = (SummaryField)get(name);
         if (field != null) return field;
         if (inherited().isEmpty()) return null;
-        return inherited().get().getSummaryField(name);
+        for (var inheritedSummary : inherited()) {
+            var inheritedField = inheritedSummary.getSummaryField(name);
+            if (inheritedField != null)
+                return inheritedField;
+        }
+        return null;
     }
 
     public Map<String, SummaryField> getSummaryFields() {
-        var fields = new LinkedHashMap<String, SummaryField>(getFields().size());
-        inherited().ifPresent(inherited -> fields.putAll(inherited.getSummaryFields()));
+        var allFields = new LinkedHashMap<String, SummaryField>(getFields().size());
+        for (var inheritedSummary : inherited()) {
+            if (inheritedSummary == null) continue;
+            allFields.putAll(inheritedSummary.getSummaryFields());
+        }
         for (var field : getFields())
-            fields.put(field.getName(), (SummaryField) field);
-        return fields;
+            allFields.put(field.getName(), (SummaryField) field);
+        return allFields;
     }
 
     /**
@@ -99,14 +108,14 @@ public class DocumentSummary extends FieldView {
         }
     }
 
-    /** Sets the parent of this. Both summaries must be present in the same search definition */
-    public void setInherited(String inherited) {
-        this.inherited = Optional.of(inherited);
+    /** Adds a parent of this. Both summaries must be present in the same schema, or a parent schema. */
+    public void addInherited(String inherited) {
+        this.inherited.add(inherited);
     }
 
     /** Returns the parent of this, if any */
-    public Optional<DocumentSummary> inherited() {
-        return inherited.map(name -> owner.getSummary(name));
+    public List<DocumentSummary> inherited() {
+        return inherited.stream().map(name -> owner.getSummary(name)).toList();
     }
 
     @Override
@@ -115,16 +124,12 @@ public class DocumentSummary extends FieldView {
     }
 
     public void validate(DeployLogger logger) {
-        if (inherited.isPresent()) {
-            if ( ! owner.getSummaries().containsKey(inherited.get())) {
-                logger.log(Level.WARNING,
-                           this + " inherits " + inherited.get() + " but this" + " is not present in " + owner);
+        for (var inheritedName : inherited) {
+            var inheritedSummary = owner.getSummary(inheritedName);
+            if (inheritedSummary == null) {
+                // TODO Vespa 9: Throw IllegalArgumentException instead
                 logger.logApplicationPackage(Level.WARNING,
-                                             this + " inherits " + inherited.get() + " but this" + " is not present in " + owner);
-                // TODO: When safe, replace the above by
-                // throw new IllegalArgumentException(this + " inherits " + inherited.get() + " but this" +
-                //                                   " is not present in " + owner);
-                // ... and update SummaryTestCase.testValidationOfInheritedSummary
+                                             this + " inherits '" + inheritedName + "' but this" + " is not present in " + owner);
             }
         }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/FieldView.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/FieldView.java
index 73d699dda1b..34a7e786947 100644
--- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/FieldView.java
+++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/FieldView.java
@@ -9,7 +9,7 @@ import java.util.LinkedHashMap;
 import java.util.Map;
 
 /**
- * @author    baldersheim
+ * @author baldersheim
  */
 public class FieldView implements Serializable {
 
@@ -30,8 +30,9 @@ public class FieldView implements Serializable {
 
     /**
      * This method will add a field to a view. All fields must come from the same document type. Not enforced here.
-     * @param field The field to add.
-     * @return Itself for chaining purposes.
+     *
+     * @param field the field to add.
+     * @return itself for chaining purposes.
      */
     public FieldView add(Field field) {
         if (fields.containsKey(field.getName())) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java
index 49cd36e4bc2..6f738cfe61f 100644
--- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java
+++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java
@@ -7,6 +7,7 @@ import com.yahoo.schema.document.TypedKey;
 
 import java.io.Serializable;
 import java.util.*;
+import java.util.stream.Collectors;
 
 import static com.yahoo.text.Lowercase.toLowerCase;
 
@@ -223,23 +224,18 @@ public class SummaryField extends Field implements Cloneable, TypedKey {
         return true;
     }
 
-    private String getDestinationString()
-    {
-        StringBuilder destinationString = new StringBuilder("destinations(");
-        for (String destination : destinations) {
-            destinationString.append(destination).append(" ");
-        }
-        destinationString.append(")");
-        return destinationString.toString();
+    private String getDestinationString() {
+        return destinations.stream().map(destination -> "document summary '" + destination + "'").collect(Collectors.joining(", "));
     }
 
+    @Override
     public String toString() {
         return "summary field '" + getName() + "'";
     }
 
     /** Returns a string which aids locating this field in the source search definition */
     public String toLocateString() {
-        return "'summary " + getName() + " type " + toLowerCase(getDataType().getName()) + "' in '" + getDestinationString() + "'";
+        return "summary " + getName() + " type " + toLowerCase(getDataType().getName()) + " in " + getDestinationString();
     }
 
     @Override
diff --git a/config-model/src/main/javacc/SchemaParser.jj b/config-model/src/main/javacc/SchemaParser.jj
index 42eeabb5ac7..3109ad42062 100644
--- a/config-model/src/main/javacc/SchemaParser.jj
+++ b/config-model/src/main/javacc/SchemaParser.jj
@@ -1461,10 +1461,8 @@ void inheritsDocumentSummary(ParsedDocumentSummary documentSummary) :
     String name;
 }
 {
-    <INHERITS> name = identifierWithDash()
-    {
-        documentSummary.inherit(name);
-    }
+    <INHERITS> name = identifierWithDash() { documentSummary.inherit(name); }
+    ( <COMMA>  name = identifierWithDash() { documentSummary.inherit(name); } )*
 }
 
 /**
diff --git a/config-model/src/test/java/com/yahoo/schema/IncorrectSummaryTypesTestCase.java b/config-model/src/test/java/com/yahoo/schema/IncorrectSummaryTypesTestCase.java
index acc872f6798..f981e0fc174 100644
--- a/config-model/src/test/java/com/yahoo/schema/IncorrectSummaryTypesTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/IncorrectSummaryTypesTestCase.java
@@ -30,7 +30,7 @@ public class IncorrectSummaryTypesTestCase extends AbstractSchemaTestCase {
                             "}\n");
             fail("processing should have failed");
         } catch (RuntimeException e) {
-            assertEquals("'summary somestring type string' in 'destinations(default )' is inconsistent with 'summary somestring type int' in 'destinations(incorrect )': All declarations of the same summary field must have the same type", e.getMessage());
+            assertEquals("summary somestring type string in document summary 'default' is inconsistent with summary somestring type int in document summary 'incorrect': All declarations of the same summary field must have the same type", e.getMessage());
         }
     }
 
diff --git a/config-model/src/test/java/com/yahoo/schema/SchemaTestCase.java b/config-model/src/test/java/com/yahoo/schema/SchemaTestCase.java
index 366c78e22c6..650e2f88f2a 100644
--- a/config-model/src/test/java/com/yahoo/schema/SchemaTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/SchemaTestCase.java
@@ -119,6 +119,9 @@ public class SchemaTestCase {
                         "    field pf1 type string {" +
                         "      indexing: summary" +
                         "    }" +
+                        "    field pf2 type string {" +
+                        "      indexing: summary" +
+                        "    }" +
                         "  }" +
                         "  fieldset parent_set {" +
                         "    fields: pf1" +
@@ -139,9 +142,12 @@ public class SchemaTestCase {
                         "  onnx-model parent_model {" +
                         "    file: models/my_model.onnx" +
                         "  }" +
-                        "  document-summary parent_summary {" +
+                        "  document-summary parent_summary1 {" +
                         "    summary pf1 type string {}" +
                         "  }" +
+                        "  document-summary parent_summary2 {" +
+                        "    summary pf2 type string {}" +
+                        "  }" +
                         "  import field parentschema_ref.name as parent_imported {}" +
                         "  raw-as-base64-in-summary" +
                         "}");
@@ -170,7 +176,7 @@ public class SchemaTestCase {
                         "  onnx-model child1_model {" +
                         "    file: models/my_model.onnx" +
                         "  }" +
-                        "  document-summary child1_summary inherits parent_summary {" +
+                        "  document-summary child1_summary inherits parent_summary1 {" +
                         "    summary c1f1 type string {}" +
                         "  }" +
                         "  import field parentschema_ref.name as child1_imported {}" +
@@ -201,7 +207,7 @@ public class SchemaTestCase {
                         "  onnx-model child2_model {" +
                         "    file: models/my_model.onnx" +
                         "  }" +
-                        "  document-summary child2_summary inherits parent_summary {" +
+                        "  document-summary child2_summary inherits parent_summary1, parent_summary2 {" +
                         "    summary c2f1 type string {}" +
                         "  }" +
                         "  import field parentschema_ref.name as child2_imported {}" +
@@ -239,14 +245,15 @@ public class SchemaTestCase {
         assertNotNull(child1.onnxModels().get("child1_model"));
         assertTrue(child1.onnxModels().containsKey("parent_model"));
         assertTrue(child1.onnxModels().containsKey("child1_model"));
-        assertNotNull(child1.getSummary("parent_summary"));
+        assertNotNull(child1.getSummary("parent_summary1"));
         assertNotNull(child1.getSummary("child1_summary"));
-        assertEquals("parent_summary", child1.getSummary("child1_summary").inherited().get().getName());
-        assertTrue(child1.getSummaries().containsKey("parent_summary"));
+        assertEquals("parent_summary1", child1.getSummary("child1_summary").inherited().get(0).getName());
+        assertTrue(child1.getSummaries().containsKey("parent_summary1"));
         assertTrue(child1.getSummaries().containsKey("child1_summary"));
         assertNotNull(child1.getSummaryField("pf1"));
         assertNotNull(child1.getSummaryField("c1f1"));
         assertNotNull(child1.getExplicitSummaryField("pf1"));
+        assertNotNull(child1.getExplicitSummaryField("pf2"));
         assertNotNull(child1.getExplicitSummaryField("c1f1"));
         assertNotNull(child1.getUniqueNamedSummaryFields().get("pf1"));
         assertNotNull(child1.getUniqueNamedSummaryFields().get("c1f1"));
@@ -274,24 +281,29 @@ public class SchemaTestCase {
         assertNotNull(child2.onnxModels().get("child2_model"));
         assertTrue(child2.onnxModels().containsKey("parent_model"));
         assertTrue(child2.onnxModels().containsKey("child2_model"));
-        assertNotNull(child2.getSummary("parent_summary"));
+        assertNotNull(child2.getSummary("parent_summary1"));
+        assertNotNull(child2.getSummary("parent_summary2"));
         assertNotNull(child2.getSummary("child2_summary"));
-        assertEquals("parent_summary", child2.getSummary("child2_summary").inherited().get().getName());
-        assertTrue(child2.getSummaries().containsKey("parent_summary"));
+        assertEquals("parent_summary1", child2.getSummary("child2_summary").inherited().get(0).getName());
+        assertEquals("parent_summary2", child2.getSummary("child2_summary").inherited().get(1).getName());
+        assertTrue(child2.getSummaries().containsKey("parent_summary1"));
+        assertTrue(child2.getSummaries().containsKey("parent_summary2"));
         assertTrue(child2.getSummaries().containsKey("child2_summary"));
         assertNotNull(child2.getSummaryField("pf1"));
         assertNotNull(child2.getSummaryField("c2f1"));
         assertNotNull(child2.getExplicitSummaryField("pf1"));
         assertNotNull(child2.getExplicitSummaryField("c2f1"));
         assertNotNull(child2.getUniqueNamedSummaryFields().get("pf1"));
+        assertNotNull(child2.getUniqueNamedSummaryFields().get("pf2"));
         assertNotNull(child2.getUniqueNamedSummaryFields().get("c2f1"));
         assertNotNull(child2.temporaryImportedFields().get().fields().get("parent_imported"));
         assertNotNull(child2.temporaryImportedFields().get().fields().get("child2_imported"));
         DocumentSummary child2DefaultSummary = child2.getSummary("default");
-        assertEquals(6, child2DefaultSummary.getSummaryFields().size());
+        assertEquals(7, child2DefaultSummary.getSummaryFields().size());
         assertTrue(child2DefaultSummary.getSummaryFields().containsKey("child2_field"));
         assertTrue(child2DefaultSummary.getSummaryFields().containsKey("parent_field"));
         assertTrue(child2DefaultSummary.getSummaryFields().containsKey("pf1"));
+        assertTrue(child2DefaultSummary.getSummaryFields().containsKey("pf2"));
         assertTrue(child2DefaultSummary.getSummaryFields().containsKey("c2f1"));
         DocumentSummary child2AttributeprefetchSummary = child2.getSummary("attributeprefetch");
         assertEquals(4, child2AttributeprefetchSummary.getSummaryFields().size());
diff --git a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
index 268e0b17b24..6b60005539e 100644
--- a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
@@ -5,6 +5,7 @@ import com.yahoo.schema.parser.ParseException;
 import com.yahoo.vespa.documentmodel.DocumentSummary;
 import com.yahoo.vespa.model.test.utils.DeployLoggerStub;
 import com.yahoo.vespa.objects.FieldBase;
+import com.yahoo.yolean.Exceptions;
 import org.junit.jupiter.api.Test;
 
 import static com.yahoo.config.model.test.TestUtil.joinLines;
@@ -67,7 +68,7 @@ public class SummaryTestCase {
                      "Fields [foo2] references non-attribute fields: " +
                      "Using this summary will cause disk accesses. " +
                      "Set 'from-disk' on this summary class to silence this warning.",
-                logger.entries.get(0).message);
+                     logger.entries.get(0).message);
     }
 
     @Test
@@ -176,9 +177,13 @@ public class SummaryTestCase {
             var actualFields = testValue.summary.getSummaryFields().values().stream()
                     .map(FieldBase::getName)
                     .toList();
-            assertEquals(Optional.ofNullable(testValue.parent),
-                    testValue.summary.inherited(),
-                    testValue.summary.getName() + (testValue.parent == null ? " does not inherit anything" : " inherits " + testValue.parent.getName()));
+            if (testValue.parent != null)
+                assertEquals(testValue.parent, testValue.summary.inherited().get(0),
+                             testValue.summary.getName() + " inherits " + testValue.parent.getName());
+            else
+                assertTrue(testValue.summary.inherited().isEmpty(),
+                           testValue.summary.getName() + " does not inherit anything");
+
             assertEquals(testValue.fields, actualFields, "Summary " + testValue.summary.getName() + " has expected fields");
         });
     }
@@ -229,17 +234,89 @@ public class SummaryTestCase {
                             "}");
             DeployLoggerStub logger = new DeployLoggerStub();
             ApplicationBuilder.createFromStrings(logger, schema);
-            assertEquals("document summary 'test_summary' inherits nonesuch but this is not present in schema 'test'",
-                    logger.entries.get(0).message);
+            assertEquals("document summary 'test_summary' inherits 'nonesuch' but this is not present in schema 'test'",
+                         logger.entries.get(0).message);
             // fail("Expected failure");
         }
         catch (IllegalArgumentException e) {
+            fail();
             // assertEquals("document summary 'test_summary' inherits nonesuch but this is not present in schema 'test'",
             //             e.getMessage());
         }
     }
 
     @Test
+    void testInheritingTwoSummariesWithConflictingFieldsFails() throws ParseException {
+        try {
+            String schema = """
+                schema test {
+                  document test {
+                    field field1 type string {
+                      indexing: summary | index | attribute
+                    }
+                    field field2 type int {
+                      indexing: summary | attribute
+                    }
+                  }
+                  document-summary parent1 {
+                    summary s1 type string {
+                        source: field1
+                    }
+                  }
+                  document-summary parent2 {
+                    summary field1 type int {
+                        source: field2
+                    }
+                  }
+                  document-summary child inherits parent1, parent2 {
+                  }
+                }
+                """;
+            DeployLoggerStub logger = new DeployLoggerStub();
+            ApplicationBuilder.createFromStrings(logger, schema);
+            fail("Expected failure");
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("summary field1 type string in document summary 'default' is inconsistent with " +
+                         "summary field1 type int in document summary 'parent2': " +
+                         "All declarations of the same summary field must have the same type",
+                         Exceptions.toMessageString(e));
+        }
+    }
+
+    @Test
+    void testInheritingTwoSummariesWithNonConflictingFieldsWorks() throws ParseException {
+        String schema = """
+                schema test {
+                  document test {
+                    field field1 type string {
+                      indexing: summary | index | attribute
+                    }
+                    field field2 type int {
+                      indexing: summary | attribute
+                    }
+                  }
+                  document-summary parent1 {
+                    summary s1 type string {
+                        source: field1
+                    }
+                  }
+                  document-summary parent2 {
+                    summary field1 type string {
+                        source: field1
+                    }
+                  }
+                  document-summary child inherits parent1, parent2 {
+                  }
+                }
+                """;
+        DeployLoggerStub logger = new DeployLoggerStub();
+        ApplicationBuilder.createFromStrings(logger, schema);
+        System.out.println("logger.entries = " + logger.entries);
+        assertTrue(logger.entries.isEmpty());
+    }
+
+    @Test
     void testInheritingParentSummary() throws ParseException {
         String parent = joinLines(
                 "schema parent {" +
@@ -265,8 +342,7 @@ public class SummaryTestCase {
                         "}");
         DeployLoggerStub logger = new DeployLoggerStub();
         ApplicationBuilder.createFromStrings(logger, parent, child);
-        logger.entries.forEach(e -> System.out.println(e));
-        //assertTrue(logger.entries.isEmpty());
+        assertTrue(logger.entries.isEmpty());
     }
 
     private static class TestValue {