Skip to content

Commit

Permalink
Correcting typos and adding more tests
Browse files Browse the repository at this point in the history 
Signed-off-by: gashutos <gashutos@amazon.com>
  • Loading branch information
@gashutos
gashutos committed Sep 15, 2023
1 parent f61aba6 commit 9393aba
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 34 deletions.
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/index/IndexSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -860,9 +860,9 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
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 caste the SortField.Type
* 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 caste the SortField to gain some sort query optimizations.
* 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);
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/index/IndexSortConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ private static MultiValueMode parseMultiValueMode(String value) {

// visible for tests
final FieldSortSpec[] sortSpecs;
final boolean shouldWidenIndexSortTpe;
final boolean shouldWidenIndexSortType;

public IndexSortConfig(IndexSettings indexSettings) {
final Settings settings = indexSettings.getSettings();
Expand Down Expand Up @@ -183,7 +183,7 @@ public IndexSortConfig(IndexSettings indexSettings) {
sortSpecs[i].missingValue = missingValues.get(i);
}
}
this.shouldWidenIndexSortTpe = indexSettings.shouldWidenIndexSortType();
this.shouldWidenIndexSortType = indexSettings.shouldWidenIndexSortType();
}

/**
Expand Down Expand Up @@ -232,7 +232,7 @@ public Sort buildIndexSort(
if (fieldData == null) {
throw new IllegalArgumentException("docvalues not found for index sort field:[" + sortSpec.field + "]");
}
if (this.shouldWidenIndexSortTpe == true) {
if (this.shouldWidenIndexSortType == true) {
sortFields[i] = fieldData.wideSortField(sortSpec.missingValue, mode, null, reverse);
} else {
sortFields[i] = fieldData.sortField(sortSpec.missingValue, mode, null, reverse);
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public final SortField wideSortField(Object missingValue, MultiValueMode sortMod
// 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 caste it to LONG to support backward compatibility info stored in segment info
// 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
Expand All @@ -166,7 +166,7 @@ public final SortField wideSortField(Object missingValue, MultiValueMode sortMod
sortField.setMissingValue(source.missingObject(missingValue, reverse));
return sortField;
}
// If already more than INT, up caste not needed.
// If already more than INT, up cast not needed.
return sortField(getNumericType(), missingValue, sortMode, nested, reverse);
}

Expand Down Expand Up @@ -243,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;
}
Expand Down
60 changes: 34 additions & 26 deletions server/src/test/java/org/opensearch/index/IndexServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,21 +534,25 @@ public void testIndexSort() {
.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);
// 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);
// 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);

// 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());
}
// 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);
}

public void testIndexSortBackwardCompatible() {
Expand All @@ -557,21 +561,25 @@ public void testIndexSortBackwardCompatible() {
.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);
// 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);
// 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);

// 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());
}
// 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);
}

private static String createTestMapping(String type) {
Expand Down

0 comments on commit 9393aba

Please sign in to comment.