Move index sorting defaults to IndexSettingProviders #135321
Conversation
jordan-powers
changed the title
jordan-powers
added
WIP
:StorageEngine/Mapping
jordan-powers
changed the title
jordan-powers
force-pushed
the
include-default-index-sort
branch
from
36af3ad to
d52e967
Compare
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| builder.putList(IndexSortConfig.INDEX_SORT_FIELD_SETTING.getKey(), DataStreamTimestampFieldMapper.DEFAULT_PATH); | ||
| builder.putList(IndexSortConfig.INDEX_SORT_ORDER_SETTING.getKey(), "desc"); | ||
| builder.putList(IndexSortConfig.INDEX_SORT_MODE_SETTING.getKey(), "min"); | ||
| builder.putList(IndexSortConfig.INDEX_SORT_MISSING_SETTING.getKey(), "_last"); |
There was a problem hiding this comment.
I think mode should be max, since order is descending and this line computes the mode? https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/index/IndexSortConfig.java#L279
There was a problem hiding this comment.
Whoops, you're right, looks like I misread the condition. It's difficult to test this since data streams reject documents with multi-valued timestamps.
| DataStreamTimestampFieldMapper.DEFAULT_PATH | ||
| ); | ||
| builder.putList(IndexSortConfig.INDEX_SORT_ORDER_SETTING.getKey(), "asc", "desc"); | ||
| builder.putList(IndexSortConfig.INDEX_SORT_MODE_SETTING.getKey(), "min", "min"); |
There was a problem hiding this comment.
Similarly to below, I think this should be min, max
|
|
||
| - match: { error.type: "illegal_argument_exception" } | ||
| - match: { error.reason: "index.sort.fields:[] index.sort.order:[asc, asc], size mismatch" } | ||
| - match: { error.reason: "index.sort.field:[] index.sort.order:[asc, asc], size mismatch" } |
There was a problem hiding this comment.
Why is index.sort.fields renamed to index.sort.field in this error message?
There was a problem hiding this comment.
Ok, I see the error message now originates from another place and index setting name is incorrect was incorrect.
| "index": { | ||
| "routing_path": [ "hostname" ], | ||
| "mode": "time_series", | ||
| "sort.field": [], |
There was a problem hiding this comment.
Did an empty sort.field yield the default before? And no longer and therefor this needed to be updated?
| additionalSettings.put(PatternTextFieldMapper.DISABLE_TEMPLATING_SETTING.getKey(), true); | ||
| } | ||
|
|
||
| if (isLogsDB && isTemplateValidation == false && IndexSortConfig.INDEX_SORT_FIELD_SETTING.exists(settings) == false) { |
There was a problem hiding this comment.
I forgot ,why didn't defining the defaults out via index sort settings in IndexSortConfig? (as setting default)
Additionally could this be done in IndexMode like is done for time series index mode?
|
Closed in favor of #135886 |
4 participants
This patch moves the logic for figuring out index sorting defaults from
IndexSortConfiginto theIndexSettingProvidersfor logsdb and tsdb.Fixes #129062
This is starting to be a big PR, but it's mostly just new tests. The change to the actual logic is fairly contained.