Skip to content

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Oct 6, 2025

We optimized the VALUES aggregation for time-series queries rather than introducing a specialized version for dimension fields, and the VALUES execution disappeared from the profiler. However, the VALUES aggregation reappeared during recent testing with multi-valued dimension fields. Therefore, this change introduces dimension_values for this purpose.

To support BWC, I need to introduce VersionedExpression, which falls back to Values if the peer node or cluster does not have DimensionValues. I am open to moving this change to VersionedNamedWriteable, but I have opted to limit the scope to ES|QL for now.

@dnhatn dnhatn added the :Analytics/ES|QL AKA ESQL label Oct 6, 2025
@dnhatn dnhatn marked this pull request as ready for review October 6, 2025 19:35
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 6, 2025
@elasticsearchmachine
Copy link
Collaborator

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

return this;
} else {
// fallback to VALUES
return new Values(source(), field(), filter());
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where we fallback dimension_values to values.

@dnhatn dnhatn added the auto-backport Automatically create backport pull requests when merged label Oct 6, 2025
@kkrik-es
Copy link
Contributor

kkrik-es commented Oct 7, 2025

Nik understands the bwc part better, can comment on that.

Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Hey, can we wait just a little bit until #131108 is done? (Currently being worked on.)

This PR introduces an alternative approach to plan versioning, namely on the level of individual expressions, and we're just moving towards a world where the planner as a whole is aware of the minimum transport version. Additionally, it means that we effectively send different query plans to different nodes (and the VersionedExpression interface paves the way for more of that), which I expect will complicate the evolution of the analyzer/planner going forward.

After #131108, we won't have to make PlanStreamOutput aware of versioned named writeables, because, instead, we will be able to pass in the min transport version into TranslateTimeSeriesAggregate so that this can either create Values or DimensionValues instances depending on the min version that's supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.1 v9.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants