Provide defaults for index sort settings #135886
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pr: 135886 | ||
| summary: Provide defaults for index sort settings | ||
| area: Mapping | ||
| type: bug | ||
| issues: | ||
| - 129062 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import org.apache.lucene.search.SortField; | ||
| import org.apache.lucene.search.SortedNumericSortField; | ||
| import org.apache.lucene.search.SortedSetSortField; | ||
| import org.elasticsearch.cluster.metadata.IndexMetadata; | ||
| import org.elasticsearch.common.logging.DeprecationCategory; | ||
| import org.elasticsearch.common.logging.DeprecationLogger; | ||
| import org.elasticsearch.common.settings.Setting; | ||
|
|
@@ -25,9 +26,10 @@ | |
| import org.elasticsearch.search.lookup.SearchLookup; | ||
| import org.elasticsearch.search.sort.SortOrder; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.EnumSet; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Objects; | ||
| import java.util.function.BiFunction; | ||
| import java.util.function.Function; | ||
|
|
@@ -59,8 +61,9 @@ public final class IndexSortConfig { | |
| /** | ||
| * The list of field names | ||
| */ | ||
| public static final Setting<List<String>> INDEX_SORT_FIELD_SETTING = Setting.stringListSettingWithDefaultProvider( | ||
| "index.sort.field", | ||
| IndexSortConfigDefaults::getDefaultSortFields, | ||
| Setting.Property.IndexScope, | ||
| Setting.Property.Final, | ||
| Setting.Property.ServerlessPublic | ||
|
|
@@ -71,7 +74,7 @@ public final class IndexSortConfig { | |
| */ | ||
| public static final Setting<List<SortOrder>> INDEX_SORT_ORDER_SETTING = Setting.listSetting( | ||
| "index.sort.order", | ||
| IndexSortConfigDefaults::getDefaultSortOrder, | ||
| IndexSortConfig::parseOrderMode, | ||
| Setting.Property.IndexScope, | ||
| Setting.Property.Final, | ||
|
|
@@ -83,7 +86,7 @@ public final class IndexSortConfig { | |
| */ | ||
| public static final Setting<List<MultiValueMode>> INDEX_SORT_MODE_SETTING = Setting.listSetting( | ||
| "index.sort.mode", | ||
| IndexSortConfigDefaults::getDefaultSortMode, | ||
| IndexSortConfig::parseMultiValueMode, | ||
| Setting.Property.IndexScope, | ||
| Setting.Property.Final, | ||
|
|
@@ -95,30 +98,91 @@ public final class IndexSortConfig { | |
| */ | ||
| public static final Setting<List<String>> INDEX_SORT_MISSING_SETTING = Setting.listSetting( | ||
| "index.sort.missing", | ||
| IndexSortConfigDefaults::getDefaultSortMissing, | ||
| IndexSortConfig::validateMissingValue, | ||
| Setting.Property.IndexScope, | ||
| Setting.Property.Final, | ||
| Setting.Property.ServerlessPublic | ||
| ); | ||
|
|
||
| public static class IndexSortConfigDefaults { | ||
| public static final FieldSortSpec[] TIME_SERIES_SORT, TIMESTAMP_SORT, HOSTNAME_TIMESTAMP_SORT, HOSTNAME_TIMESTAMP_BWC_SORT; | ||
|
|
||
| static { | ||
| FieldSortSpec timeStampSpec = new FieldSortSpec(DataStreamTimestampFieldMapper.DEFAULT_PATH); | ||
| timeStampSpec.order = SortOrder.DESC; | ||
| TIME_SERIES_SORT = new FieldSortSpec[] { new FieldSortSpec(TimeSeriesIdFieldMapper.NAME), timeStampSpec }; | ||
| TIMESTAMP_SORT = new FieldSortSpec[] { timeStampSpec }; | ||
|
|
||
| FieldSortSpec hostnameSpec = new FieldSortSpec(IndexMode.HOST_NAME); | ||
| hostnameSpec.order = SortOrder.ASC; | ||
| hostnameSpec.missingValue = "_last"; | ||
| hostnameSpec.mode = MultiValueMode.MIN; | ||
| HOSTNAME_TIMESTAMP_SORT = new FieldSortSpec[] { hostnameSpec, timeStampSpec }; | ||
|
|
||
| // Older indexes use ascending ordering for host name and timestamp. | ||
| HOSTNAME_TIMESTAMP_BWC_SORT = new FieldSortSpec[] { | ||
| new FieldSortSpec(IndexMode.HOST_NAME), | ||
| new FieldSortSpec(DataStreamTimestampFieldMapper.DEFAULT_PATH) }; | ||
| } | ||
|
|
||
| public static FieldSortSpec[] getDefaultSortSpecs(Settings settings) { | ||
| if (settings.isEmpty()) { | ||
| return new FieldSortSpec[0]; | ||
| } | ||
|
|
||
| String indexMode = settings.get(IndexSettings.MODE.getKey()); | ||
|
There was a problem hiding this comment. Can't use There was a problem hiding this comment. Maybe add this also as a comment? |
||
| if (indexMode != null) { | ||
| indexMode = indexMode.toLowerCase(Locale.ROOT); | ||
| } | ||
|
|
||
| if (IndexMode.TIME_SERIES.getName().equals(indexMode)) { | ||
| return TIME_SERIES_SORT; | ||
| } else if (IndexMode.LOGSDB.getName().equals(indexMode)) { | ||
| var version = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings); | ||
| if (version.onOrAfter(IndexVersions.LOGSB_OPTIONAL_SORTING_ON_HOST_NAME) | ||
| || version.between( | ||
| IndexVersions.LOGSB_OPTIONAL_SORTING_ON_HOST_NAME_BACKPORT, | ||
| IndexVersions.UPGRADE_TO_LUCENE_10_0_0 | ||
| )) { | ||
| return (IndexSettings.LOGSDB_SORT_ON_HOST_NAME.get(settings)) ? HOSTNAME_TIMESTAMP_SORT : TIMESTAMP_SORT; | ||
| } else { | ||
| return HOSTNAME_TIMESTAMP_BWC_SORT; | ||
| } | ||
| } | ||
|
|
||
| return new FieldSortSpec[0]; | ||
| } | ||
|
|
||
| public static List<String> getDefaultSortFields(Settings settings) { | ||
| return Arrays.stream(getDefaultSortSpecs(settings)).map(sortSpec -> sortSpec.field).toList(); | ||
| } | ||
|
|
||
| public static List<String> getDefaultSortOrder(Settings settings) { | ||
|
There was a problem hiding this comment. I believe there are situations where non-default and default settings can be mixed incorrectly. For example, if mode=logsdb, and only a sort field is set, the order, mode, missing settings will use the defaults for timestamp sort. this test (added to 80_index_sort_defaults.yml) shows the issue: There was a problem hiding this comment. Good catch. The current logic does ensure the defaults aren't actually applied, but they're still reported via the settings api. I'll update the logic so that if a custom sort is defined, the defaults match There was a problem hiding this comment. Ok, should be fixed as of b8ff892 |
||
| return Arrays.stream(getDefaultSortSpecs(settings)) | ||
| .map(sortSpec -> sortSpec.order != null ? sortSpec.order : SortOrder.ASC) | ||
| .map(Enum::toString) | ||
| .toList(); | ||
| } | ||
|
|
||
| public static List<String> getDefaultSortMode(Settings settings) { | ||
| return Arrays.stream(getDefaultSortSpecs(settings)).map(sortSpec -> { | ||
| if (sortSpec.mode != null) { | ||
| return sortSpec.mode; | ||
| } else if (sortSpec.order == SortOrder.DESC) { | ||
| return MultiValueMode.MAX; | ||
| } else { | ||
| return MultiValueMode.MIN; | ||
| } | ||
| }).map(order -> order.toString().toLowerCase(Locale.ROOT)).toList(); | ||
| } | ||
|
|
||
| public static List<String> getDefaultSortMissing(Settings settings) { | ||
| // _last is the default per IndexFieldData.XFieldComparatorSource.Nested#sortMissingLast | ||
| return Arrays.stream(getDefaultSortSpecs(settings)) | ||
| .map(sortSpec -> sortSpec.missingValue != null ? sortSpec.missingValue : "_last") | ||
| .toList(); | ||
| } | ||
| } | ||
|
|
||
| private static String validateMissingValue(String missing) { | ||
|
|
@@ -146,6 +210,41 @@ private static MultiValueMode parseMultiValueMode(String value) { | |
| return mode; | ||
| } | ||
|
|
||
| private static void checkSizeMismatch(String firstKey, List<?> first, String secondKey, List<?> second) { | ||
| if (first.size() != second.size()) { | ||
| throw new IllegalArgumentException(firstKey + ":" + first + " " + secondKey + ":" + second + ", size mismatch"); | ||
| } | ||
| } | ||
|
|
||
| private static void validateSortSettings(Settings settings) { | ||
| if (INDEX_SORT_FIELD_SETTING.exists(settings) == false) { | ||
| for (Setting<?> setting : new Setting<?>[] { INDEX_SORT_ORDER_SETTING, INDEX_SORT_MODE_SETTING, INDEX_SORT_MISSING_SETTING }) { | ||
| if (setting.exists(settings)) { | ||
| throw new IllegalArgumentException( | ||
| "setting [" + setting.getKey() + "] requires [" + INDEX_SORT_FIELD_SETTING.getKey() + "] to be configured" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| List<String> fields = INDEX_SORT_FIELD_SETTING.get(settings); | ||
|
|
||
| if (INDEX_SORT_ORDER_SETTING.exists(settings)) { | ||
| var order = INDEX_SORT_ORDER_SETTING.get(settings); | ||
| checkSizeMismatch(INDEX_SORT_FIELD_SETTING.getKey(), fields, INDEX_SORT_ORDER_SETTING.getKey(), order); | ||
| } | ||
|
|
||
| if (INDEX_SORT_MODE_SETTING.exists(settings)) { | ||
| var mode = INDEX_SORT_MODE_SETTING.get(settings); | ||
| checkSizeMismatch(INDEX_SORT_FIELD_SETTING.getKey(), fields, INDEX_SORT_MODE_SETTING.getKey(), mode); | ||
| } | ||
|
|
||
| if (INDEX_SORT_MISSING_SETTING.exists(settings)) { | ||
| var missing = INDEX_SORT_MISSING_SETTING.get(settings); | ||
| checkSizeMismatch(INDEX_SORT_FIELD_SETTING.getKey(), fields, INDEX_SORT_MISSING_SETTING.getKey(), missing); | ||
| } | ||
| } | ||
|
|
||
| // visible for tests | ||
| final FieldSortSpec[] sortSpecs; | ||
| private final IndexVersion indexCreatedVersion; | ||
|
|
@@ -158,37 +257,13 @@ public IndexSortConfig(IndexSettings indexSettings) { | |
| this.indexName = indexSettings.getIndex().getName(); | ||
| this.indexMode = indexSettings.getMode(); | ||
|
|
||
| validateSortSettings(settings); | ||
|
|
||
| List<String> fields = INDEX_SORT_FIELD_SETTING.get(settings); | ||
| boolean applyDefaults = INDEX_SORT_FIELD_SETTING.exists(settings) == false; | ||
| sortSpecs = fields.stream().map(FieldSortSpec::new).toArray(FieldSortSpec[]::new); | ||
|
|
||
| if (INDEX_SORT_ORDER_SETTING.exists(settings) || applyDefaults) { | ||
| List<SortOrder> orders = INDEX_SORT_ORDER_SETTING.get(settings); | ||
| if (orders.size() != sortSpecs.length) { | ||
| throw new IllegalArgumentException("index.sort.field:" + fields + " index.sort.order:" + orders + ", size mismatch"); | ||
|
|
@@ -198,7 +273,7 @@ public IndexSortConfig(IndexSettings indexSettings) { | |
| } | ||
| } | ||
|
|
||
| if (INDEX_SORT_MODE_SETTING.exists(settings) || applyDefaults) { | ||
| List<MultiValueMode> modes = INDEX_SORT_MODE_SETTING.get(settings); | ||
| if (modes.size() != sortSpecs.length) { | ||
| throw new IllegalArgumentException("index.sort.field:" + fields + " index.sort.mode:" + modes + ", size mismatch"); | ||
|
|
@@ -208,7 +283,7 @@ public IndexSortConfig(IndexSettings indexSettings) { | |
| } | ||
| } | ||
|
|
||
| if (INDEX_SORT_MISSING_SETTING.exists(settings) || applyDefaults) { | ||
| List<String> missingValues = INDEX_SORT_MISSING_SETTING.get(settings); | ||
| if (missingValues.size() != sortSpecs.length) { | ||
| throw new IllegalArgumentException( | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bug, but I'm not sure why we never saw it before. Maybe we never had string list settings whose default value depended on other settings before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, definitely looks like a bug to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, I agree that the current behaviour isn't correct and actual settings need to be pushed down instead of default settings..