Provide defaults for index sort settings by jordan-powers · Pull Request #135886 · elastic/elasticsearch

jordan-powers · 2025-10-03T06:17:18Z

This PR configures default values for the index settings index.sort.field, index.sort.order, index.sort.mode, and index.sort.missing.

Fixes #129062

elasticsearchmachine · 2025-10-03T06:17:45Z

Hi @jordan-powers, I've created a changelog YAML for you.

…ex-sort-2

jordan-powers · 2025-10-03T07:06:21Z

server/src/main/java/org/elasticsearch/index/IndexSortConfig.java

+                return new FieldSortSpec[0];
+            }
+
+            String indexMode = settings.get(IndexSettings.MODE.getKey());


Can't use IndexSettings.MODE.get(settings) here because the validation logic for IndexSettings.MODE uses the default value of index.sort.*, which causes infinite recursion (since we're already in the default value provider for those settings).
So we need to get the mode while bypassing the validation.

Maybe add this also as a comment?

jordan-powers · 2025-10-03T07:09:48Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

                List<String> asList = defaultSettings.getAsList(getKey(), null);
                if (asList == null) {
-                    builder.putList(getKey(), defaultStringValue.apply(defaultSettings));
+                    builder.putList(getKey(), defaultStringValue.apply(source));


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?

Nice find, definitely looks like a bug to me

Good find, I agree that the current behaviour isn't correct and actual settings need to be pushed down instead of default settings..

elasticsearchmachine · 2025-10-03T15:32:08Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

parkertimmins · 2025-10-03T16:36:55Z

server/src/main/java/org/elasticsearch/index/IndexSortConfig.java

+            return Arrays.stream(getDefaultSortSpecs(settings)).map(sortSpec -> sortSpec.field).toList();
+        }
+
+        public static List<String> getDefaultSortOrder(Settings settings) {


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:

--- create logsdb data stream with non-default sort field: - do: cluster.put_component_template: name: "logsdb-mappings" body: template: settings: mode: "logsdb" index.sort.field: ["some_field"] mappings: properties: some_field: type: "keyword" "@timestamp": type: "date" - do: indices.put_index_template: name: "logsdb-index-template" body: index_patterns: ["logsdb"] data_stream: {} composed_of: ["logsdb-mappings"] allowed_warnings: - "index template [logsdb-index-template] has index patterns [logsdb] matching patterns from existing older templates [global] with patterns (global => [*]); this template [logsdb-index-template] will take precedence during new index creation" - do: indices.create_data_stream: name: "logsdb" - is_true: acknowledged - do: indices.get_data_stream: name: "logsdb" expand_wildcards: hidden - length: { data_streams: 1 } - set: { data_streams.0.indices.0.index_name: backing_index } - do: indices.get_settings: index: $backing_index include_defaults: true - match: { .$backing_index.settings.index.mode: logsdb } - match: { .$backing_index.settings.index.sort.field: [ "some_field" ] } - match: { .$backing_index.defaults.index.sort.order: [ "asc" ] } - match: { .$backing_index.defaults.index.sort.mode: [ "min" ] } - match: { .$backing_index.defaults.index.sort.missing: [ "_last" ] }

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 index.sort.fields.

Ok, should be fixed as of b8ff892

…ex-sort-2

parkertimmins

LGTM!

martijnvg

This looks good Jordan! I left a few questions.

martijnvg · 2025-10-07T07:31:03Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

                List<String> asList = defaultSettings.getAsList(getKey(), null);
                if (asList == null) {
-                    builder.putList(getKey(), defaultStringValue.apply(defaultSettings));
+                    builder.putList(getKey(), defaultStringValue.apply(source));


Good find, I agree that the current behaviour isn't correct and actual settings need to be pushed down instead of default settings..

martijnvg · 2025-10-07T07:35:10Z

server/src/main/java/org/elasticsearch/index/IndexSortConfig.java

+                return new FieldSortSpec[0];
+            }
+
+            String indexMode = settings.get(IndexSettings.MODE.getKey());


Maybe add this also as a comment?

martijnvg · 2025-10-07T07:42:22Z

...ck/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/30_logsdb_default_mapping.yml

  - match: { .$backing_index.mappings.properties.@timestamp.type: date }
-  - match: { .$backing_index.mappings.properties.host.properties.name.type: keyword }
-  - match: { .$backing_index.mappings.properties.host.properties.name.ignore_above: 1024 }
+  - match: { .$backing_index.mappings.properties.host: null }


Why does this need to be changed? iirc these fields get injected by default if index mode is logsdb?

The update I made to LogsdbIndexModeSettingsProvider was causing this test to fail.

We don't inject the host.name mapper when there is a custom sort configured. In this test, we configure index.sort.field to be an empty array. Previously, we checked for a custom sort by checking if index.sort.field is non-empty. We can't do that anymore because index.sort.field now has a proper default and will be sometimes non-empty even when there is no custom sort configured. Now, we check for a custom sort by actually checking if index.sort.field is configured.

martijnvg · 2025-10-07T07:46:56Z

x-pack/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/80_index_sort_defaults.yml

+  - match: { .$backing_index.defaults.index.sort.mode: [ "max" ] }
+  - match: { .$backing_index.defaults.index.sort.missing: [ "_last" ] }
+---
+create logsdb data stream with host as text and name as double:


This is a strange setup, but I see how this can work and using host.name as primary sort field.

Yeah it's definitely a strange setup, but I found it tested in 30_logsdb_default_mapping, and I figured it couldn't hurt to test the edge case.

martijnvg · 2025-10-07T07:54:20Z

...sdb/src/javaRestTest/java/org/elasticsearch/xpack/logsdb/LogsIndexModeEnabledRestTestIT.java

                    "mode": "time_series",
-                    "sort.field": [],
-                    "sort.order": []
+                    "sort.field": ["_tsid", "@timestamp"],


Shouldn't the sort.field / sort.order index settings be derived from the defaults? Or maybe the settings shouldn't be defined in this test?

Omitting the sort.field and sort.order results in the error updating component template [logs@custom] results in invalid composable template [logs-apm.error@template] after templates are merged.

From what I can tell, the logs-apm.error@template template is composed of logs@custom which we define here, and apm@settings which contains a non-default index sort configuration. Since this this template sets the index mode to time_series, we need to override the index sort from apm@settings with another index sort here.

…ex-sort-2

elasticsearchmachine · 2025-10-08T20:16:49Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts
❌	9.2	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 135886

jordan-powers · 2025-10-09T16:49:22Z

After some discussion, we decided not to backport this.

This PR optimizes the default providers for the sort settings index.sort.field, index.sort.order, index.sort.mode, and index.sort.missing to fix a performance regression in the many-shards-quantitative benchmark associated with the introduction of these default providers in #135886.

jordan-powers added 3 commits October 2, 2025 21:43

Add default providers for index sort settings

502f0c9

Update tests

4d95872

Fix tests

52921ee

jordan-powers self-assigned this Oct 3, 2025

jordan-powers added >bug auto-backport Automatically create backport pull requests when merged Team:StorageEngine :StorageEngine/Mapping The storage related side of mappings v8.19.6 v9.1.6 v9.0.9 v9.2.1 v9.3.0 labels Oct 3, 2025

jordan-powers and others added 5 commits October 2, 2025 23:17

Update docs/changelog/135886.yaml

c6b6c12

Add yaml test for tsdb index sorting defaults

e0ee6ab

Fix tests

e13b33d

Merge remote-tracking branch 'upstream/main' into include-default-ind…

3687453

…ex-sort-2

Reorder members in IndexSortConfig

cb4f9f2

jordan-powers commented Oct 3, 2025

View reviewed changes

jordan-powers assigned martijnvg and parkertimmins Oct 3, 2025

jordan-powers marked this pull request as ready for review October 3, 2025 15:31

jordan-powers requested a review from a team as a code owner October 3, 2025 15:31

jordan-powers mentioned this pull request Oct 3, 2025

Move index sorting defaults to IndexSettingProviders #135321

Closed

parkertimmins requested changes Oct 3, 2025

View reviewed changes

jordan-powers added 2 commits October 3, 2025 15:41

Fix default sort values for non-default sorts

b8ff892

Merge remote-tracking branch 'upstream/main' into include-default-ind…

947e785

…ex-sort-2

Fix typo

d8dc3e8

parkertimmins self-requested a review October 6, 2025 16:59

parkertimmins approved these changes Oct 6, 2025

View reviewed changes

martijnvg reviewed Oct 7, 2025

View reviewed changes

jordan-powers added 3 commits October 7, 2025 08:16

Merge remote-tracking branch 'upstream/main' into include-default-ind…

72926e0

…ex-sort-2

Add comment to IndexSortConfig

f7ff485

Merge remote-tracking branch 'upstream/main' into include-default-ind…

c073bb7

…ex-sort-2

jordan-powers unassigned martijnvg and parkertimmins Oct 8, 2025

jordan-powers merged commit 9fe91ff into elastic:main Oct 8, 2025
34 checks passed

elasticsearchmachine added the backport pending label Oct 8, 2025

jordan-powers removed backport pending auto-backport Automatically create backport pull requests when merged v8.19.6 v9.1.6 v9.0.9 v9.2.1 labels Oct 9, 2025

This was referenced Nov 27, 2025

Optimize sort settings default providers #138711

Merged

Cache default settings #138712

Closed

jordan-powers deleted the include-default-index-sort-2 branch December 13, 2025 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide defaults for index sort settings#135886

Provide defaults for index sort settings#135886
jordan-powers merged 14 commits intoelastic:mainfrom
jordan-powers:include-default-index-sort-2

jordan-powers commented Oct 3, 2025

elasticsearchmachine commented Oct 3, 2025

jordan-powers Oct 3, 2025

martijnvg Oct 7, 2025

jordan-powers Oct 3, 2025

parkertimmins Oct 3, 2025

martijnvg Oct 7, 2025

elasticsearchmachine commented Oct 3, 2025

parkertimmins Oct 3, 2025

jordan-powers Oct 3, 2025

jordan-powers Oct 3, 2025

parkertimmins left a comment

martijnvg left a comment

martijnvg Oct 7, 2025

martijnvg Oct 7, 2025

martijnvg Oct 7, 2025

jordan-powers Oct 7, 2025

martijnvg Oct 7, 2025

jordan-powers Oct 7, 2025

martijnvg Oct 7, 2025

jordan-powers Oct 7, 2025

Uh oh!

elasticsearchmachine commented Oct 8, 2025

jordan-powers commented Oct 9, 2025

Labels

4 participants

Conversation

jordan-powers commented Oct 3, 2025

elasticsearchmachine commented Oct 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkertimmins left a comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 8, 2025

💔 Backport failed

jordan-powers commented Oct 9, 2025

Labels

4 participants