Skip to content

Conversation

@jordan-powers
Copy link
Contributor

@jordan-powers jordan-powers commented Feb 16, 2025

This PR exends the work done in #122161 and #121751 to also use the doc values sparse index for the _tsid fields in time-series mode indices.

@jordan-powers jordan-powers self-assigned this Feb 16, 2025
@jordan-powers jordan-powers changed the title Tsid sparse doc values Doc values sparse index on _tsid fields Feb 16, 2025
@jordan-powers jordan-powers added the test-full-bwc Trigger full BWC version matrix tests label Feb 18, 2025
@jordan-powers jordan-powers marked this pull request as ready for review February 18, 2025 18:59
@elasticsearchmachine
Copy link
Collaborator

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

context.doc().add(new SortedDocValuesField(fieldType().name(), timeSeriesId));

if (this.useDocValuesSkipper && this.indexCreatedVersion.onOrAfter(IndexVersions.TIME_SERIES_ID_DOC_VALUES_SPARSE_INDEX)) {
context.doc().add(SortedDocValuesField.indexedField(fieldType().name(), timeSeriesId));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SortedDocValuesField#indexedField creates a new SortedDocValuesField using a private static FieldType that has the skip index enabled. It's the same as discussed for SortedNumericDocValuesField in #122161 (comment)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jordan-powers! I left two comments.

public static final String NAME = "_tsid";
public static final String CONTENT_TYPE = "_tsid";
public static final TimeSeriesIdFieldType FIELD_TYPE = new TimeSeriesIdFieldType();
public static final TimeSeriesIdFieldMapper INSTANCE = new TimeSeriesIdFieldMapper();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why this is a singleton constant is to reduce memory usage of MapperService. Meta field mappers that can be initialized as a constant are a constant. A node can have many MapperService instances (one per allocated index).

I think can just define two singleton instances here and then decide to use which in IndexMode#timeSeriesIdFieldMapper(...) based on index versions and whether the feature flag is enabled?


private TimeSeriesIdFieldMapper() {
private final IndexVersion indexCreatedVersion;
private final boolean useDocValuesSkipper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid passing down index version here. We can set useDocValuesSkipper based on the following expressions: indexVersion.onOrAfter(IndexVersions.TIME_SERIES_ID_DOC_VALUES_SPARSE_INDEX) && context.getIndexSettings().useDocValuesSkipper()?

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jordan-powers jordan-powers merged commit bf31ee6 into elastic:main Feb 19, 2025
17 checks passed
@jordan-powers jordan-powers deleted the tsid-sparse-doc-values branch February 19, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :StorageEngine/TSDB You know, for Metrics Team:StorageEngine test-full-bwc Trigger full BWC version matrix tests v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants