diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java index 5ecac5e4078..1a85f6229db 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java @@ -22,6 +22,7 @@ import org.apache.jackrabbit.oak.plugins.index.AsyncIndexInfoService; import org.apache.jackrabbit.oak.plugins.index.IndexEditorProvider; import org.apache.jackrabbit.oak.plugins.index.IndexInfoProvider; +import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticDocument; import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticIndexEditorProvider; import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticRetryPolicy; import org.apache.jackrabbit.oak.plugins.index.elastic.query.ElasticIndexProvider; @@ -236,6 +237,9 @@ private void activate(BundleContext bundleContext, Config config) { oakRegs.add(whiteboard.register(FeatureToggle.class, new FeatureToggle(ElasticIndexStatistics.FT_OAK_12248, ElasticIndexStatistics.FT_OAK_12248_ENABLE), emptyMap())); + oakRegs.add(whiteboard.register(FeatureToggle.class, + new FeatureToggle(ElasticDocument.FT_OAK_12353, ElasticDocument.FT_OAK_12353_ENABLE), + emptyMap())); if (System.getProperty(QueryEngineSettings.OAK_INFERENCE_ENABLED) != null) { this.isInferenceEnabled = Boolean.parseBoolean(System.getProperty(QueryEngineSettings.OAK_INFERENCE_ENABLED)); } else { diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocument.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocument.java index e6012c6d881..ec7fe820c3a 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocument.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocument.java @@ -35,13 +35,27 @@ import java.util.Map; import java.util.HashMap; import java.util.Set; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Collectors; import static org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexUtils.toFloats; @JsonInclude(JsonInclude.Include.NON_EMPTY) public class ElasticDocument { + public static final String FT_OAK_12353 = "FT_OAK-12353"; + /** + * When {@code true}, dynamic boost values sharing the same boost score are grouped into a + * single nested document per property, with {@code value} holding an array of the grouped + * values, instead of one nested document per value. This reduces the number of nested + * documents generated for properties with many dynamic-boost values that share a boost score. + * Default is {@code true} (feature enabled); set to {@code false} via the feature toggle to + * revert to the pre-fix behaviour of one nested document per value. + */ + public static final AtomicBoolean FT_OAK_12353_ENABLE = new AtomicBoolean(true); + @JsonProperty(FieldNames.PATH) public final String path; @JsonProperty(ElasticIndexDefinition.PATH_RANDOM_VALUE) @@ -66,6 +80,12 @@ public class ElasticDocument { @JsonProperty(ElasticIndexDefinition.LAST_UPDATED) private long lastUpdated; + // fieldName -> boost -> values sharing that boost. Only populated when FT_OAK_12353_ENABLE is + // true, in which case it replaces the corresponding entries that would otherwise be added to + // "properties" directly by addDynamicBoostField. + @JsonIgnore + private final Map>> dynamicBoostGroups; + // Internal set with properties that need to be removed from the document on update operations @JsonIgnore private final Set propertiesToRemove; @@ -87,6 +107,7 @@ public class ElasticDocument { this.dbFullText = new LinkedHashSet<>(); this.similarityTags = new LinkedHashSet<>(); this.propertiesToRemove = new HashSet<>(); + this.dynamicBoostGroups = new LinkedHashMap<>(); } void addFulltext(String value) { @@ -175,12 +196,18 @@ void indexAncestors(String path) { } void addDynamicBoostField(String fieldName, String value, double boost) { - addProperty(fieldName, - Map.of( - ElasticIndexHelper.DYNAMIC_BOOST_NESTED_VALUE, value, - ElasticIndexHelper.DYNAMIC_BOOST_NESTED_BOOST, boost - ) - ); + if (FT_OAK_12353_ENABLE.get()) { + dynamicBoostGroups.computeIfAbsent(fieldName, k -> new LinkedHashMap<>()) + .computeIfAbsent(boost, k -> new LinkedHashSet<>()) + .add(value); + } else { + addProperty(fieldName, + Map.of( + ElasticIndexHelper.DYNAMIC_BOOST_NESTED_VALUE, value, + ElasticIndexHelper.DYNAMIC_BOOST_NESTED_BOOST, boost + ) + ); + } // add value into the dynamic boost specific fulltext field. We cannot add this in the standard // field since dynamic boosted terms require lower weight compared to standard terms @@ -197,7 +224,21 @@ void setLastUpdated(long lastUpdated) { @JsonAnyGetter public Map getProperties() { - return properties; + if (dynamicBoostGroups.isEmpty()) { + return properties; + } + Map merged = new LinkedHashMap<>(properties); + dynamicBoostGroups.forEach((fieldName, boostToValues) -> { + Set nestedDocs = boostToValues.entrySet().stream() + .map(entry -> Map.of( + ElasticIndexHelper.DYNAMIC_BOOST_NESTED_VALUE, + entry.getValue().size() == 1 ? entry.getValue().iterator().next() : entry.getValue(), + ElasticIndexHelper.DYNAMIC_BOOST_NESTED_BOOST, entry.getKey() + )) + .collect(Collectors.toCollection(LinkedHashSet::new)); + merged.put(fieldName, nestedDocs); + }); + return merged; } public void removeProperty(String fieldName) { @@ -222,6 +263,9 @@ public String toString() { if (!dynamicProperties.isEmpty()) { buff.append("dynamicProperties:").append(dynamicProperties).append('\n'); } + if (!dynamicBoostGroups.isEmpty()) { + buff.append("dynamicBoostGroups:").append(dynamicBoostGroups).append('\n'); + } return buff.toString(); } diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java index fe484d7b1f6..32b488231ff 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java @@ -393,7 +393,7 @@ private static void checkIndexRules(ElasticIndexDefinition indexDefinition) { .stream() .filter(e -> e.getValue().size() > 1) .filter(e -> e.getValue().stream().map(PropertyDefinition::getType).distinct().count() > 1) - .collect(Collectors.toList()); + .toList(); if (!multiTypesFields.isEmpty()) { String fields = multiTypesFields.stream().map(Map.Entry::getKey).collect(Collectors.joining(", ", "[", "]")); @@ -417,7 +417,7 @@ protected static String convertUpperCamelToLowerUnderscore(String string) { StringBuilder result = new StringBuilder(); for (char c : string.toCharArray()) { // start? - if (result.length() == 0) { + if (result.isEmpty()) { result.append(Character.toLowerCase(c)); } else { if (Character.isUpperCase(c)) { diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostTest.java index 28cc5e999d2..bdfd5868254 100644 --- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostTest.java +++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostTest.java @@ -20,20 +20,48 @@ import org.apache.jackrabbit.oak.api.ContentRepository; import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.plugins.index.DynamicBoostCommonTest; +import org.apache.jackrabbit.oak.plugins.index.elastic.index.ElasticDocument; +import org.junit.After; +import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import java.util.List; +/** + * Runs every test twice, once with {@link ElasticDocument#FT_OAK_12353_ENABLE} enabled and once + * with it disabled, so both dynamic-boost grouping code paths get the same coverage. + */ +@RunWith(Parameterized.class) public class ElasticDynamicBoostTest extends DynamicBoostCommonTest { @ClassRule public static final ElasticConnectionRule elasticRule = new ElasticConnectionRule(); + @Parameterized.Parameters(name = "dynamicBoostGroupingEnabled={0}") + public static Iterable fixtures() { + return List.of(new Object[]{true}, new Object[]{false}); + } + + @Parameterized.Parameter + public boolean dynamicBoostGroupingEnabled; + public ElasticDynamicBoostTest() { this.indexOptions = new ElasticIndexOptions(); } + @Before + public void setDynamicBoostGroupingToggle() { + ElasticDocument.FT_OAK_12353_ENABLE.set(dynamicBoostGroupingEnabled); + } + + @After + public void resetDynamicBoostGroupingToggle() { + ElasticDocument.FT_OAK_12353_ENABLE.set(true); + } + @Override protected ContentRepository createRepository() { repositoryOptionsUtil = new ElasticTestRepositoryBuilder(elasticRule).build(); @@ -85,6 +113,34 @@ public void dynamicBoostAnalyzed() throws Exception { }); } + /** + * Predicted tags sharing the same boost score are grouped into a single nested document + * (see {@link ElasticDocument#FT_OAK_12353_ENABLE}). This verifies that querying still + * matches on any of the grouped values, both with the grouping enabled and disabled + * (see {@link #dynamicBoostGroupingEnabled}). + */ + @Test + public void dynamicBoostQueriesGroupedValuesSharingSameBoostScore() throws Exception { + createAssetsIndexAndProperties(false, false); + + Tree testParent = createNodeWithType(root.getTree("/"), "test", JcrConstants.NT_UNSTRUCTURED, ""); + + Tree predicted1 = createAssetNodeWithPredicted(testParent, "asset1", "flower with a lot of red and a bit of blue"); + createPredictedTag(predicted1, "red", 5.0); + createPredictedTag(predicted1, "blue", 5.0); + createPredictedTag(predicted1, "green", 5.0); + createPredictedTag(predicted1, "special", 9.0); + + root.commit(); + + assertEventually(() -> { + assertQuery("//element(*, dam:Asset)[jcr:contains(., 'red')]", XPATH, List.of("/test/asset1")); + assertQuery("//element(*, dam:Asset)[jcr:contains(., 'blue')]", XPATH, List.of("/test/asset1")); + assertQuery("//element(*, dam:Asset)[jcr:contains(., 'green')]", XPATH, List.of("/test/asset1")); + assertQuery("//element(*, dam:Asset)[jcr:contains(., 'special')]", XPATH, List.of("/test/asset1")); + }); + } + @Test public void dynamicBoostNotIncludedInFullText() throws Exception { createAssetsIndexAndProperties(false, false, false); @@ -119,4 +175,27 @@ public void dynamicBoostNotIncludedInFullText() throws Exception { }); } + + @Test + public void ranking() throws Exception { + createAssetsIndexAndProperties(false, false); + Tree test = createNodeWithType(root.getTree("/"), "test", JcrConstants.NT_UNSTRUCTURED, ""); + + // asset1: three tags sharing one boost group (boost 1) + Tree many = createAssetNodeWithPredicted(test, "asset1", "titleone"); + createPredictedTag(many, "red", 1.0); + createPredictedTag(many, "blue", 1.0); + createPredictedTag(many, "green", 1.0); + + // asset2: one high-boost tag in its own group, the other two effectively zero + Tree single = createAssetNodeWithPredicted(test, "asset2", "titletwo"); + createPredictedTag(single, "red", 4.0); + createPredictedTag(single, "blue", 0.01); + createPredictedTag(single, "green", 0.01); + + root.commit(); + String query = + "select [jcr:path] from [dam:Asset] where contains(*, 'red blue green')"; + assertEventually(() -> assertOrderedQuery(query, List.of("/test/asset2", "/test/asset1"))); + } } diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentTest.java new file mode 100644 index 00000000000..09c142d49d4 --- /dev/null +++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentTest.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.plugins.index.elastic.index; + +import org.junit.After; +import org.junit.Test; + +import java.time.LocalDate; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class ElasticDocumentTest { + + @After + public void resetToggle() { + ElasticDocument.FT_OAK_12353_ENABLE.set(true); + } + + @Test + public void dynamicBoostValuesAreNotGroupedWhenToggleDisabled() { + ElasticDocument.FT_OAK_12353_ENABLE.set(false); + + ElasticDocument doc = new ElasticDocument("/test"); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "Replacement Cost", 1.0); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "Theft", 1.0); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "GENERAL INSURANCE COMPANY", 0.988); + + Object value = doc.getProperties().get("predictedTagsDynamicBoost"); + assertTrue(value instanceof Set); + @SuppressWarnings("unchecked") + Set> nestedDocs = (Set>) value; + assertEquals(3, nestedDocs.size()); + for (Map nestedDoc : nestedDocs) { + assertTrue(nestedDoc.get(ElasticIndexHelper.DYNAMIC_BOOST_NESTED_VALUE) instanceof String); + } + } + + @Test + public void dynamicBoostValuesAreGroupedByBoostByDefault() { + ElasticDocument doc = new ElasticDocument("/test"); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "Replacement Cost", 1.0); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "Theft", 1.0); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "Alberta", 1.0); + doc.addDynamicBoostField("predictedTagsDynamicBoost", "GENERAL INSURANCE COMPANY", 0.988); + + Object value = doc.getProperties().get("predictedTagsDynamicBoost"); + assertTrue(value instanceof Set); + @SuppressWarnings("unchecked") + Set> nestedDocs = (Set>) value; + // one nested doc for the 3 values sharing boost=1.0, one for the distinct boost=0.988 + assertEquals(2, nestedDocs.size()); + + boolean foundGrouped = false; + boolean foundSingle = false; + for (Map nestedDoc : nestedDocs) { + Object boost = nestedDoc.get(ElasticIndexHelper.DYNAMIC_BOOST_NESTED_BOOST); + Object nestedValue = nestedDoc.get(ElasticIndexHelper.DYNAMIC_BOOST_NESTED_VALUE); + if (Double.valueOf(1.0).equals(boost)) { + assertTrue(nestedValue instanceof Collection); + @SuppressWarnings("unchecked") + Collection values = (Collection) nestedValue; + assertEquals(List.of("Replacement Cost", "Theft", "Alberta"), new ArrayList<>(values)); + foundGrouped = true; + } else if (Double.valueOf(0.988).equals(boost)) { + assertEquals("GENERAL INSURANCE COMPANY", nestedValue); + foundSingle = true; + } + } + assertTrue(foundGrouped); + assertTrue(foundSingle); + } + + @Test + public void ft_oak_12353_toggleShouldBeRemoved() { + // Time-bombed: if this test fails, the feature toggle FT_OAK-12353 and its guard in + // ElasticDocument#addDynamicBoostField/#getProperties should be removed — the grouping + // has been enabled by default in production long enough. + assertTrue("Feature toggle " + ElasticDocument.FT_OAK_12353 + " is overdue for removal", + LocalDate.now().isBefore(LocalDate.of(2027, 8, 12))); + } +} diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/FulltextIndexConstants.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/FulltextIndexConstants.java index a71df247b56..ebbb55014dc 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/FulltextIndexConstants.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/FulltextIndexConstants.java @@ -485,6 +485,6 @@ public static IndexingMode from(String indexingMode) { * needed from an outside process that does not have visibility to the specific index module. */ Map INDEX_VERSION_BY_TYPE = Map.of( - "elasticsearch", "1.4.0" + "elasticsearch", "1.5.0" ); }