Skip to content

Support non-keyword dimensions as routing fields in TSDB#105501

Merged
kkrik-es merged 64 commits intoelastic:mainfrom
kkrik-es:fix/103567-3
Mar 21, 2024
Merged

Support non-keyword dimensions as routing fields in TSDB#105501
kkrik-es merged 64 commits intoelastic:mainfrom
kkrik-es:fix/103567-3

Conversation

@kkrik-es
Copy link
Copy Markdown
Member

@kkrik-es kkrik-es commented Feb 14, 2024

This is a follow-up on #103648, lifting the limitation that the pass-through object can contain only keyword fields if it's marked as time_series_dimension. In particular, fields included in the routing path no more need to be keywords.

#106080 included all required changes around routing. This change builds on top of it, including changes in mappings. In particular, it streamlines mapping eligibility as dimension fields.

Related to #103567

@kkrik-es kkrik-es self-assigned this Feb 14, 2024
@kkrik-es
Copy link
Copy Markdown
Member Author

kkrik-es commented Mar 7, 2024

This is getting pretty long, I'm forking the _ts_routing_hash and routing logic to #106080. Once that's in, this one will be used just to update the mappings.

# Conflicts:
#	modules/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java
#	server/src/main/java/org/elasticsearch/cluster/routing/IndexRouting.java
#	server/src/main/java/org/elasticsearch/index/IndexMode.java
#	server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java
#	server/src/main/java/org/elasticsearch/index/mapper/TimeSeriesRoutingHashFieldMapper.java
#	server/src/main/java/org/elasticsearch/index/mapper/TsidExtractingIdFieldMapper.java
#	server/src/test/java/org/elasticsearch/index/mapper/TimeSeriesIdFieldMapperTests.java
#	server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java
@kkrik-es kkrik-es changed the title Introduce TimeSeriesRoutingIdFieldMapper to support non-keyword dimensions Support non-keyword dimensions in TSDB Mar 13, 2024
@kkrik-es
Copy link
Copy Markdown
Member Author

The forked PR is in, back to this. Changes are now limited to mappings, mostly.

@martijnvg martijnvg added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Mar 14, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Mar 14, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search (Team:Search)

Comment thread docs/changelog/105501.yaml Outdated
super(name);
}

void setDimension() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe rename this method to enableDimension()?

Copy link
Copy Markdown
Member Author

@kkrik-es kkrik-es Mar 19, 2024

Choose a reason for hiding this comment

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

Hm that would be misleading, we want to mark that the mapper to be built will actually be a dimension, not that the builder is enabled to build a dimension (it is, de factor).

I really don't like the name though. This class creates objects that can build dimension fields but will not necessarily do. Any suggestions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice if we can move Parameter<Boolean> dimension into this builder, but that looks like a bigger refactoring to me.

So passthrough field mapper invokes setDimension() here and then other field mappers (keyword, number, etc.) can become dimensions. Maybe this method should be called inheritDimension()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Alright, switched to inheritDimensionParameterFromParentObject, more verbose for sure.

Comment thread server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java Outdated
@kkrik-es kkrik-es changed the title Support non-keyword dimensions in TSDB Support non-keyword dimensions as routing fields in TSDB Mar 19, 2024
Copy link
Copy Markdown
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types :StorageEngine/TSDB You know, for Metrics Team:Search Meta label for search team Team:StorageEngine v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants