ES|QL: fix stats by constant expresson with alias

[luigidellaquila]

Fix planning of queries with STATS and grouping aliases

FROM test
| EVAL y = \"a\"
| STATS COUNT() BY x=y
| SORT x

x is now excluded from the references list (it's a generated value), so that the index resolution happens correctly.

Fixes: #114714

[elasticsearchmachine]

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

[elasticsearchmachine]

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

[luigidellaquila]

Plans have logic to correctly calculate their references, no need to reinvent the wheel here.

In general, IMHO this method is trying to do too much.
The logic should be simple: each plan should declare needed inputs and produced outputs; this method should just start from the top of the plan, visit it down and for each plan node

• remove the outputs
• add the required inputs.

All the instanceof above should be encapsulated in the single logical plan classes.

[alex-spies]

Ideally, yes. I was about to suggest a refactor of fieldNames to make things simpler and more local, similarly to what you described.

However, since we're at a point here where references are not yet resolved, we cannot use the existing query plan methods as we would wish. Esp. the plan's output and outputSet methods do not work here because these require resolved references, and also need to know about what fields are even present in relations. This, and the fact that we need to match references by name rather than by id. This prevents us from just using the simple approach from ProjectAwayColumns.

However, using the references method should be okay at this point because that's something that can be determined locally for a plan node.

[alex-spies]

Nice fix. It turns out, it's the same problem with accidentally accounting for groupings in the Aggregate's references, but in a different place because we use a manual way to determine a plan's output in fieldNames.

[alex-spies]

Ideally, yes. I was about to suggest a refactor of fieldNames to make things simpler and more local, similarly to what you described.

However, since we're at a point here where references are not yet resolved, we cannot use the existing query plan methods as we would wish. Esp. the plan's output and outputSet methods do not work here because these require resolved references, and also need to know about what fields are even present in relations. This, and the fact that we need to match references by name rather than by id. This prevents us from just using the simple approach from ProjectAwayColumns.

However, using the references method should be okay at this point because that's something that can be determined locally for a plan node.

[astefan]

Thanks for having a second look at this issue @luigidellaquila.
The big advantage of fieldNames is that it can be unit tested. Please, add some tests (be creative with queries that can break the logic in fieldNames) in IndexResolverFieldNamesTests.

[astefan]

Awesome. Thank you for adding the unit tests, @luigidellaquila
LGTM

[luigidellaquila]

Thanks for the reviews @astefan @alex-spies !

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.17	Commit could not be cherrypicked due to conflicts
❌	8.16	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 117551

[luigidellaquila]

Backporting manually