Skip to content

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 19, 2025

This change updates the METRICS command syntax from METRICS index_pattern aggregations | ... to METRICS index_pattern | ..., removing inline aggregations. This PR does not filter out non-time-series indexes from the source; this will be addressed in a follow-up PR to keep this PR small.

@dnhatn dnhatn requested review from costin, kkrik-es and martijnvg March 19, 2025 15:13
@dnhatn dnhatn marked this pull request as ready for review March 19, 2025 15:13
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine labels Mar 19, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@Override
protected LogicalPlan rule(EsRelation r) {
if (r.indexMode() == IndexMode.TIME_SERIES) {
if (r.output().stream().anyMatch(a -> a.name().equals(MetadataAttribute.TSID_FIELD)) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

Due to their perf caracteristics, we're looking to move away from the streams - use
Expressions.anyMatch(r.output(), a -> MetadataAttributes.TSID_FIELD.equals(a.name()) == false) instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, pushed 3453f98

/**
* Uses the standard index mode if the special index mode is not required in the query.
*/
public final class DropIndexMode extends OptimizerRules.OptimizerRule<EsRelation> {
Copy link
Member

Choose a reason for hiding this comment

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

how about PruneUnusedIndexMode ?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed in aa3a26c

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.

👍

null
);
return new Aggregate(source, relation, Aggregate.AggregateType.METRICS, stats.groupings, stats.aggregates);
return visitRelation(source(ctx), "METRICS", ctx.indexPatternAndMetadataFields());
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to say this, but this may get renamed again (there's some confusion with the name "metrics"..). Still, I think the rest of this PR should hold and this should be a much smaller change (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's correct.

EMPTY,
unresolvedTSRelation("foo"),
Aggregate.AggregateType.METRICS,
Aggregate.AggregateType.STANDARD,
Copy link
Contributor

Choose a reason for hiding this comment

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

For my education, don't we want to track all aggs right after METRICS as AggregateType.METRICS? In the sense that they're applied per-tsid by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not using AggregateType now. We can adjust it later when we start using it.

Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

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

That was quick, thanks Nhat.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 20, 2025

Thanks friends!

@dnhatn dnhatn merged commit c8de8d1 into elastic:main Mar 20, 2025
16 of 17 checks passed
@dnhatn dnhatn deleted the metrics-command branch March 20, 2025 07:12
kanoshiou added a commit to kanoshiou/elasticsearch that referenced this pull request Mar 21, 2025
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025
This change updates the METRICS command syntax from METRICS 
index_pattern aggregations | ... to METRICS index_pattern | ...,
removing inline aggregations. This PR does not filter out
non-time-series indexes from the source; this will be addressed in a
follow-up PR to keep this PR small.
omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025
This change updates the METRICS command syntax from METRICS 
index_pattern aggregations | ... to METRICS index_pattern | ...,
removing inline aggregations. This PR does not filter out
non-time-series indexes from the source; this will be addressed in a
follow-up PR to keep this PR small.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants