Skip to content

Conversation

@astefan
Copy link
Contributor

@astefan astefan commented Sep 26, 2025

This accounts for synthetics attributes that are added as part of union types support inside StubRelation (an essential component of inline stats support). StubRelation can be regarded as a source plan node (similar to EsRelation) and, thus, the synthetics attributes handling as part of union types feature should be mirrored in this "source" plan as well.

To make this handling "encapsulated" and tightly linked to StubRelation, the constructor of the class has been changed so that the output list of Attributes is computed only in one place and be "reused" whenever a StubRelation is created or changed.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 26, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @astefan, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

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

@astefan astefan requested a review from alex-spies September 28, 2025 04:22
@astefan astefan removed the request for review from bpintea September 29, 2025 10:45
@alex-spies alex-spies removed their request for review September 30, 2025 08:39
Copy link
Contributor

@craigtaverner craigtaverner left a comment

Choose a reason for hiding this comment

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

LGTM

summary: Fix union types lost attributes in `StubRelation` for inlinestats
area: ES|QL
type: bug
issues: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Might have been nice to have an issue describing this more

DENSE_VECTOR_AGG_METRIC_DOUBLE_IF_FNS
DENSE_VECTOR_AGG_METRIC_DOUBLE_IF_FNS,

INLINE_STATS_WITH_UNION_TYPES_IN_STUB_RELATION(INLINE_STATS.enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should move up in the list to the rest of the INLINE_STATS capabilities. We try to avoid adding to the end of the capabilities, because that causes increased conflict probability. It's also nicer when exploring for all related Capabilities to be near each other.

| INLINE STATS c = count(emp_no::long) where hire_date > "1991-01-01" BY yr
| SORT yr DESC
| LIMIT 5
| SORT yr DESC, hire_date DESC
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice all these fixed tests involve a grouping key. Do we have tests for non-grouped inline stats?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I added a test here for this, and it worked. I'll push a commit for it, and if you feel that test is redundant, just revert the commit.

@astefan astefan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 30, 2025
@astefan
Copy link
Contributor Author

astefan commented Sep 30, 2025

Thank you, @craigtaverner, for the review and updates to the PR!

@elasticsearchmachine elasticsearchmachine merged commit 1560c79 into elastic:main Sep 30, 2025
34 checks passed
@astefan astefan deleted the fix_union_types_lost_attributes branch September 30, 2025 12:39
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-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants