Fix broken backward compatibility from 2.7 for IndexSorted field indi…

…ces (opensearch-project#10045) * Fix broken backward comparibility from 2.7 for IndexSorted field indices Signed-off-by: gashutos <gashutos@amazon.com> * Adding CHANGELOG Signed-off-by: gashutos <gashutos@amazon.com> * Update server/src/main/java/org/opensearch/index/IndexSettings.java Co-authored-by: Andriy Redko <drreta@gmail.com> Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> * Removing unwanted logs Signed-off-by: gashutos <gashutos@amazon.com> * Removing unwanted logs Signed-off-by: gashutos <gashutos@amazon.com> * Adding index sort as part of mixed cluster to test this scenario Signed-off-by: gashutos <gashutos@amazon.com> * Removing optimization disable logic Signed-off-by: gashutos <gashutos@amazon.com> * Correcting some comments & version check to before( V_2_7_0) instead onOrBefire(V_2_6_1) since Signed-off-by: gashutos <gashutos@amazon.com> * Resolving spotless check error Signed-off-by: gashutos <gashutos@amazon.com> * Fixing broken UT - last minute checkin without copile Signed-off-by: gashutos <gashutos@amazon.com> * Improving code coverage to make codcov happy Signed-off-by: gashutos <gashutos@amazon.com> * Correcting typos and adding more tests Signed-off-by: gashutos <gashutos@amazon.com> * Removing unwanted imports Signed-off-by: gashutos <gashutos@amazon.com> --------- Signed-off-by: gashutos <gashutos@amazon.com> Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> Co-authored-by: Andriy Redko <drreta@gmail.com>
vikasvb90 · Oct 10, 2023 · 9b39fbf · 9b39fbf
1 parent 223a3f9
commit 9b39fbf
Show file tree

Hide file tree

Showing 8 changed files with 173 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -100,6 +100,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ### Fixed
 - Fix ignore_missing parameter has no effect when using template snippet in rename ingest processor ([#9725](https://github.com/opensearch-project/OpenSearch/pull/9725))
+- Fix broken backward compatibility from 2.7 for IndexSorted field indices ([#10045](https://github.com/opensearch-project/OpenSearch/pull/9725))
 
 ### Security
 

diff --git a/qa/mixed-cluster/src/test/java/org/opensearch/backwards/IndexingIT.java b/qa/mixed-cluster/src/test/java/org/opensearch/backwards/IndexingIT.java
@@ -67,13 +67,14 @@
 public class IndexingIT extends OpenSearchRestTestCase {
 
     protected static final Version UPGRADE_FROM_VERSION = Version.fromString(System.getProperty("tests.upgrade_from_version"));
+    private static final String TEST_MAPPING = createTestMapping();
 
 
     private int indexDocs(String index, final int idStart, final int numDocs) throws IOException {
         for (int i = 0; i < numDocs; i++) {
             final int id = idStart + i;
             Request request = new Request("PUT", index + "/_doc/" + id);
-            request.setJsonEntity("{\"test\": \"test_" + randomAlphaOfLength(2) + "\"}");
+            request.setJsonEntity("{\"test\": \"test_" + randomAlphaOfLength(2) + "\", \"sortfield\": \""+ randomIntBetween(0, numDocs) + "\"}");
             assertOK(client().performRequest(request));
         }
         return numDocs;
@@ -129,9 +130,10 @@ public void testIndexingWithPrimaryOnBwcNodes() throws Exception {
             .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1)
             .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0)
             .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT)
+            .putList("index.sort.field", "sortfield")
             .put("index.routing.allocation.include._name", bwcNames);
         final String index = "test-index";
-        createIndex(index, settings.build());
+        createIndex(index, settings.build(), TEST_MAPPING);
         ensureNoInitializingShards(); // wait for all other shard activity to finish
 
         int docCount = 200;
@@ -178,9 +180,10 @@ public void testIndexingWithReplicaOnBwcNodes() throws Exception {
             .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1)
             .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0)
             .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT)
+            .putList("index.sort.field", "sortfield")
             .put("index.routing.allocation.exclude._name", bwcNames);
         final String index = "test-index";
-        createIndex(index, settings.build());
+        createIndex(index, settings.build(), TEST_MAPPING);
         ensureNoInitializingShards(); // wait for all other shard activity to finish
         printClusterRouting();
 
@@ -214,11 +217,12 @@ public void testIndexVersionPropagation() throws Exception {
         Settings.Builder settings = Settings.builder()
                 .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1)
                 .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 2)
+                .putList("index.sort.field", "sortfield")
                 .put("index.routing.allocation.include._name", bwcNames);
         final String index = "indexversionprop";
         final int minUpdates = 5;
         final int maxUpdates = 10;
-        createIndex(index, settings.build());
+        createIndex(index, settings.build(), TEST_MAPPING);
         try (RestClient newNodeClient = buildClient(restClientSettings(),
                 nodes.getNewNodes().stream().map(Node::getPublishAddress).toArray(HttpHost[]::new))) {
 
@@ -300,10 +304,11 @@ public void testSeqNoCheckpoints() throws Exception {
         Settings.Builder settings = Settings.builder()
             .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1)
             .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 2)
+            .putList("index.sort.field", "sortfield")
             .put("index.routing.allocation.include._name", bwcNames);
 
         final String index = "test";
-        createIndex(index, settings.build());
+        createIndex(index, settings.build(), TEST_MAPPING);
         try (RestClient newNodeClient = buildClient(restClientSettings(),
             nodes.getNewNodes().stream().map(Node::getPublishAddress).toArray(HttpHost[]::new))) {
             int numDocs = 0;
@@ -382,10 +387,11 @@ public void testUpdateSnapshotStatus() throws Exception {
         Settings.Builder settings = Settings.builder()
             .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), between(5, 10))
             .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 1)
+            .putList("index.sort.field", "sortfield")
             .put("index.routing.allocation.include._name", bwcNames);
 
         final String index = "test-snapshot-index";
-        createIndex(index, settings.build());
+        createIndex(index, settings.build(), TEST_MAPPING);
         indexDocs(index, 0, between(50, 100));
         ensureGreen(index);
         assertOK(client().performRequest(new Request("POST", index + "/_refresh")));
@@ -419,7 +425,8 @@ public void testSyncedFlushTransition() throws Exception {
         createIndex(index, Settings.builder()
             .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), numShards)
             .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numOfReplicas)
-            .put("index.routing.allocation.include._name", newNodes).build());
+            .putList("index.sort.field", "sortfield")
+            .put("index.routing.allocation.include._name", newNodes).build(), TEST_MAPPING);
         ensureGreen(index);
         indexDocs(index, randomIntBetween(0, 100), between(1, 100));
         try (RestClient oldNodeClient = buildClient(restClientSettings(),
@@ -664,4 +671,15 @@ public String toString() {
                 '}';
         }
     }
+
+    private static String createTestMapping() {
+        return "  \"properties\": {\n"
+            + "    \"test\": {\n"
+            + "      \"type\": \"text\"\n"
+            + "    },\n"
+            + "    \"sortfield\": {\n"
+            + "      \"type\": \"integer\"\n"
+            + "    }\n"
+            + "  }";
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java
@@ -63,6 +63,7 @@
 import java.util.function.Function;
 import java.util.function.UnaryOperator;
 
+import static org.opensearch.Version.V_2_7_0;
 import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY;
 import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING;
 import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_FIELD_NAME_LENGTH_LIMIT_SETTING;
@@ -660,6 +661,7 @@ public final class IndexSettings {
     private volatile long retentionLeaseMillis;
 
     private volatile String defaultSearchPipeline;
+    private final boolean widenIndexSortType;
 
     /**
      * The maximum age of a retention lease before it is considered expired.
@@ -857,6 +859,13 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
         mergeOnFlushEnabled = scopedSettings.get(INDEX_MERGE_ON_FLUSH_ENABLED);
         setMergeOnFlushPolicy(scopedSettings.get(INDEX_MERGE_ON_FLUSH_POLICY));
         defaultSearchPipeline = scopedSettings.get(DEFAULT_SEARCH_PIPELINE);
+        /* There was unintentional breaking change got introduced with [OpenSearch-6424](https://github.com/opensearch-project/OpenSearch/pull/6424) (version 2.7).
+         * For indices created prior version (prior to 2.7) which has IndexSort type, they used to type cast the SortField.Type
+         * to higher bytes size like integer to long. This behavior was changed from OpenSearch 2.7 version not to
+         * up cast the SortField to gain some sort query optimizations.
+         * Now this sortField (IndexSort) is stored in SegmentInfo and we need to maintain backward compatibility for them.
+         */
+        widenIndexSortType = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings).before(V_2_7_0);
 
         scopedSettings.addSettingsUpdateConsumer(MergePolicyConfig.INDEX_COMPOUND_FORMAT_SETTING, mergePolicyConfig::setNoCFSRatio);
         scopedSettings.addSettingsUpdateConsumer(
@@ -1652,4 +1661,12 @@ public String getDefaultSearchPipeline() {
     public void setDefaultSearchPipeline(String defaultSearchPipeline) {
         this.defaultSearchPipeline = defaultSearchPipeline;
     }
+
+    /**
+     * Returns true if we need to maintain backward compatibility for index sorted indices created prior to version 2.7
+     * @return boolean
+     */
+    public boolean shouldWidenIndexSortType() {
+        return this.widenIndexSortType;
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/IndexSortConfig.java b/server/src/main/java/org/opensearch/index/IndexSortConfig.java
@@ -143,6 +143,7 @@ private static MultiValueMode parseMultiValueMode(String value) {
 
     // visible for tests
     final FieldSortSpec[] sortSpecs;
+    final boolean shouldWidenIndexSortType;
 
     public IndexSortConfig(IndexSettings indexSettings) {
         final Settings settings = indexSettings.getSettings();
@@ -182,6 +183,7 @@ public IndexSortConfig(IndexSettings indexSettings) {
                 sortSpecs[i].missingValue = missingValues.get(i);
             }
         }
+        this.shouldWidenIndexSortType = indexSettings.shouldWidenIndexSortType();
     }
 
     /**
@@ -230,7 +232,11 @@ public Sort buildIndexSort(
             if (fieldData == null) {
                 throw new IllegalArgumentException("docvalues not found for index sort field:[" + sortSpec.field + "]");
             }
-            sortFields[i] = fieldData.sortField(sortSpec.missingValue, mode, null, reverse);
+            if (this.shouldWidenIndexSortType == true) {
+                sortFields[i] = fieldData.wideSortField(sortSpec.missingValue, mode, null, reverse);
+            } else {
+                sortFields[i] = fieldData.sortField(sortSpec.missingValue, mode, null, reverse);
+            }
             validateIndexSortField(sortFields[i]);
         }
         return new Sort(sortFields);

diff --git a/server/src/main/java/org/opensearch/index/fielddata/IndexFieldData.java b/server/src/main/java/org/opensearch/index/fielddata/IndexFieldData.java
@@ -94,6 +94,13 @@ public interface IndexFieldData<FD extends LeafFieldData> {
      */
     SortField sortField(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse);
 
+    /**
+     * Returns the {@link SortField} to use for index sorting where we widen the sort field type to higher or equal bytes.
+     */
+    default SortField wideSortField(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) {
+        return sortField(missingValue, sortMode, nested, reverse);
+    }
+
     /**
      * Build a sort implementation specialized for aggregations.
      */

diff --git a/server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java b/server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java
@@ -151,6 +151,25 @@ public final SortField sortField(Object missingValue, MultiValueMode sortMode, N
         return sortField(getNumericType(), missingValue, sortMode, nested, reverse);
     }
 
+    @Override
+    public final SortField wideSortField(Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) {
+        // This is to support backward compatibility, the minimum number of bytes prior to OpenSearch 2.7 were 16 bytes,
+        // i.e all sort fields were upcasted to Long/Double with 16 bytes.
+        // Now from OpenSearch 2.7, the minimum number of bytes for sort field is 8 bytes, so if it comes as SortField INT,
+        // we need to up cast it to LONG to support backward compatibility info stored in segment info
+        if (getNumericType().sortFieldType == SortField.Type.INT) {
+            XFieldComparatorSource source = comparatorSource(NumericType.LONG, missingValue, sortMode, nested);
+            SortedNumericSelector.Type selectorType = sortMode == MultiValueMode.MAX
+                ? SortedNumericSelector.Type.MAX
+                : SortedNumericSelector.Type.MIN;
+            SortField sortField = new SortedNumericSortField(getFieldName(), SortField.Type.LONG, reverse, selectorType);
+            sortField.setMissingValue(source.missingObject(missingValue, reverse));
+            return sortField;
+        }
+        // If already more than INT, up cast not needed.
+        return sortField(getNumericType(), missingValue, sortMode, nested, reverse);
+    }
+
     /**
      * Builds a {@linkplain BucketedSort} for the {@code targetNumericType},
      * casting the values if their native type doesn't match.
@@ -224,7 +243,7 @@ private XFieldComparatorSource comparatorSource(
                 source = new IntValuesComparatorSource(this, missingValue, sortMode, nested);
         }
         if (targetNumericType != getNumericType()) {
-            source.disableSkipping(); // disable skipping logic for caste of sort field
+            source.disableSkipping(); // disable skipping logic for cast of sort field
         }
         return source;
     }

diff --git a/server/src/test/java/org/opensearch/index/IndexServiceTests.java b/server/src/test/java/org/opensearch/index/IndexServiceTests.java
@@ -33,7 +33,9 @@
 package org.opensearch.index;
 
 import org.apache.lucene.search.MatchAllDocsQuery;
+import org.apache.lucene.search.SortField;
 import org.apache.lucene.search.TopDocs;
+import org.opensearch.Version;
 import org.opensearch.action.support.ActiveShardCount;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.compress.CompressedXContent;
@@ -526,4 +528,76 @@ public void testUpdateRemoteTranslogBufferIntervalDynamically() {
         indexMetadata = client().admin().cluster().prepareState().execute().actionGet().getState().metadata().index("test");
         assertEquals("20s", indexMetadata.getSettings().get(IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING.getKey()));
     }
+
+    public void testIndexSort() {
+        Settings settings = Settings.builder()
+            .put(IndexSettings.INDEX_TRANSLOG_SYNC_INTERVAL_SETTING.getKey(), "0ms") // disable
+            .putList("index.sort.field", "sortfield")
+            .build();
+        try {
+            // Integer index sort should be remained to int sort type
+            IndexService index = createIndex("test", settings, createTestMapping("integer"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.INT);
+
+            // Long index sort should be remained to long sort type
+            index = createIndex("test", settings, createTestMapping("long"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
+
+            // Float index sort should be remained to float sort type
+            index = createIndex("test", settings, createTestMapping("float"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.FLOAT);
+
+            // Double index sort should be remained to double sort type
+            index = createIndex("test", settings, createTestMapping("double"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.DOUBLE);
+
+            // String index sort should be remained to string sort type
+            index = createIndex("test", settings, createTestMapping("string"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.STRING);
+        } catch (IllegalArgumentException ex) {
+            assertEquals("failed to parse value [0ms] for setting [index.translog.sync_interval], must be >= [100ms]", ex.getMessage());
+        }
+    }
+
+    public void testIndexSortBackwardCompatible() {
+        Settings settings = Settings.builder()
+            .put(IndexSettings.INDEX_TRANSLOG_SYNC_INTERVAL_SETTING.getKey(), "0ms") // disable
+            .put(IndexMetadata.SETTING_INDEX_VERSION_CREATED.getKey(), Version.V_2_6_1)
+            .putList("index.sort.field", "sortfield")
+            .build();
+        try {
+            // Integer index sort should be converted to long sort type
+            IndexService index = createIndex("test", settings, createTestMapping("integer"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
+
+            // Long index sort should be remained to long sort type
+            index = createIndex("test", settings, createTestMapping("long"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.LONG);
+
+            // Float index sort should be remained to float sort type
+            index = createIndex("test", settings, createTestMapping("float"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.FLOAT);
+
+            // Double index sort should be remained to double sort type
+            index = createIndex("test", settings, createTestMapping("double"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.DOUBLE);
+
+            // String index sort should be remained to string sort type
+            index = createIndex("test", settings, createTestMapping("string"));
+            assertTrue(index.getIndexSortSupplier().get().getSort()[0].getType() == SortField.Type.STRING);
+        } catch (IllegalArgumentException ex) {
+            assertEquals("failed to parse value [0ms] for setting [index.translog.sync_interval], must be >= [100ms]", ex.getMessage());
+        }
+    }
+
+    private static String createTestMapping(String type) {
+        return "  \"properties\": {\n"
+            + "    \"test\": {\n"
+            + "      \"type\": \"text\"\n"
+            + "    },\n"
+            + "    \"sortfield\": {\n"
+            + "      \"type\": \" + type + \"\n"
+            + "    }\n"
+            + "  }";
+    }
 }
diff --git a/server/src/test/java/org/opensearch/index/fielddata/AbstractFieldDataImplTestCase.java b/server/src/test/java/org/opensearch/index/fielddata/AbstractFieldDataImplTestCase.java
@@ -39,6 +39,7 @@
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Sort;
 import org.apache.lucene.search.SortField;
+import org.apache.lucene.search.SortedNumericSortField;
 import org.apache.lucene.search.TopFieldDocs;
 import org.apache.lucene.util.BytesRef;
 import org.opensearch.core.common.Strings;
@@ -144,6 +145,27 @@ public void testSingleValueAllSet() throws Exception {
         }
     }
 
+    public void testWideSortField() throws Exception {
+        if (this instanceof NoOrdinalsStringFieldDataTests || this instanceof PagedBytesStringFieldDataTests) {
+            return; // Numeric types are not supported there.
+        }
+        // integer to long widening should happen
+        IndexFieldData<?> indexFieldData = getForField("int", "value");
+        SortField sortField = indexFieldData.wideSortField(null, MultiValueMode.MIN, null, false);
+        assertTrue(((SortedNumericSortField) sortField).getNumericType() == SortField.Type.LONG);
+
+        // long to long no widening should happen
+        indexFieldData = getForField("long", "value");
+        sortField = indexFieldData.wideSortField(null, MultiValueMode.MIN, null, false);
+        assertTrue(((SortedNumericSortField) sortField).getNumericType() == SortField.Type.LONG);
+
+        // float to float no widening should happen
+        indexFieldData = getForField("float", "value");
+        sortField = indexFieldData.wideSortField(null, MultiValueMode.MIN, null, false);
+        assertTrue(((SortedNumericSortField) sortField).getNumericType() == SortField.Type.FLOAT);
+
+    }
+
     protected abstract void fillSingleValueWithMissing() throws Exception;
 
     public void assertValues(SortedBinaryDocValues values, int docId, BytesRef... actualValues) throws IOException {