ESQL: Have CombineProjections propagate references upwards

[bpintea]

This will have CombineProjections allow references from the "under" plan be kept when merging stacked Projections.
This is to prevent field attributes that are dropped by ReplaceMissingFieldWithNull be used in the resulting plan.

[elasticsearchmachine]

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

[elasticsearchmachine]

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

[costin]

Nice find.

[elasticsearchmachine]

💔 Backport failed

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

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

[alex-spies]

Heya, I don't understand what problem this is solving. @bpintea , could you elaborate why it's bad if dropped field attributes get referenced upstream? They're not really dropped, they're actually shadowed by a reference attribute with the same name id obtained by injecting an Eval, but the planning shouldn't (so far) care about that.

Because, this behavior is not entirely eradicated, as can be seen in your added test (thanks!), where the exchange still thinks it's handling a field attribute emp_no, while the projection just below it knows it's handling a reference attribute emp_no in fact.

This is an inherent wonkiness in ReplaceMissingFieldWithNull, which I think can't be solved unless we stop injecting Evals in that logical optimizer rule, and instead rely on field extraction being efficient for missing fields, which'd be a less brittle long term fix.

[bpintea]

Hey @alex-spies,

They're not really dropped, they're actually shadowed by a reference attribute with the same name

Indeed, you're technically correct. :)

this behavior is not entirely eradicated, as can be seen in your added test (thanks!), where the exchange still thinks it's handling a field attribute

That happens on the coordinator, right? Which should be correct, as that's the plan output there. The shadowing with null only happens on the remote. I'd say that output there is an artefact of the testing setup.

why it's bad if dropped field attributes get referenced upstream?

Merging the stack plans and selecting the field attribute - which is in that case shadowed - is simply incorrect and was so just because the rule only considered aliases for the AttributeMap. The fact that it worked (so far) doesn't make it correct.

Laterally, I got it by revisiting the rules, not a failed query.

[alex-spies]

Merging the stack plans and selecting the field attribute - which is in that case shadowed - is simply incorrect and was so just because the rule only considered aliases for the AttributeMap. The fact that it worked (so far) doesn't make it correct.

I agree with you that this should be considered incorrect that we're referencing a field attribute although we're actually using a reference attribute obtained from an EVAL. It only works because the attributes use the same name id, which is an abuse of the fact that physically, only name ids and types matter, not full attributes.

Unfortunately, it's been working like this since ReplaceMissingFieldsWithNull's inception, I believe. This PR also only fixes this weirdness for some Project nodes (specifically, only those that get affected by CombineProjections), while all other upstream nodes referencing the field attribute are not updated.

Fixing this properly would require updating the expressions of all downstream plans after the Eval injected by ReplaceMissingFieldsWithNull; we'd need to replace the field attr by the new ref attr everywhere. When I recently worked on ReplaceMissingFieldsWithNull, I initially wanted to do just this; but some plans may genuinely require a field attribute's information to work, so that approach can't work in general and I abandoned it. (I considered a bunch of approaches for my recent fix of the opt. rule, c.f. PR description here)

If we really want to properly fix this, I think it should be fixed by acknowledging that ReplaceMissingFieldsWithNull is trying to perform a physical optimization when it injects EVALs in case of missing fields; and we should just get rid of the Eval injection part and instead make InsertFieldExtractions inject operations that create null blocks (which is equivalent).

[bpintea]

This PR also only fixes this weirdness for some Project nodes (specifically, only those that get affected by CombineProjections)

Correct (as per the PR's title :) ); it's a small fix.

while all other upstream nodes referencing the field attribute are not updated.

True; that would be a larger, potentially different change: I guess subsequently in some of those cases we could/should fold the null further or drop parts of the plan (à la #125577).

[costin]

this is an optimization, unrelated to this PR, right?

[bpintea]

Correct. It's supposed to avoid creating objects while planning, if not needed. The change is small and not sure if a stand-alone PR would make sense, but happy to revert and split, if needed.

[costin]

This captures aliases but now also References - why do we care about them and what about other types of named expression (e.g. FieldAttribute)?

[alex-spies]

My understanding is that this change doesn't affect queries and their computation, it just opportunistically replaces e.g. emp_no{f} by emp_no{r} (with same name id) in some projections if there's something like

FROM employees | KEEP emp_no

but emp_no is missing and ReplaceMissingFieldsWithNull injects an EVAL emp_no = null after the FROM.

I don't think this makes a difference, really, as the attribute type mismatch between emp_no{f} and emp_no{r} remains in most other commands in the query (but this is, currently, just a "cosmetic" issue in the planning).

[bpintea]

it just opportunistically replaces e.g. emp_no{f} by emp_no{r} (with same name id) in some projections if there's something like

It's not just then. There'll always be two projections to combine in case of a locally missing field, due to the effects of ProjectAwayColumns and ReplaceMissingFieldWithNull both creating Project nodes.

I don't think this makes a difference, really, as the attribute type mismatch between emp_no{f} and emp_no{r} remains in most other commands in the query (but this is, currently, just a "cosmetic" issue in the planning).

IMO the change the PR proposes is a correction, not just "cosmetic". But it's true that it currently makes no practical difference.
But to exemplify why this works now: FROM ... | EVAL x = missing_field + 1 gets planned as Eval - Limit - EsRelation. We're pushing Eval down as much as possible, but we push Limit "harder". With the effect that the LimitExec "breaks" the plan and the local fragment lacks the Eval, which makes the extraction work (i.e. actually skip the field).
But ideally this evaluation would be distributed and one day Eval pushed into the fragment as well. The InsertFieldExtraction only looks at FieldAttributes (and MetadataAttributes), so we we'd want to have the merge generate a ReferenceAttribute instead of a field one.

This captures aliases but now also References - why do we care about them and what about other types of named expression (e.g. FieldAttribute)?

I hope the above clarifies. But happy to give further details, if needed.

But I'm also happy to revert the change and add a comment instead. :)

[alex-spies]

InsertFieldExtraction only looks at FieldAttributes (and MetadataAttributes), so we we'd want to have the merge generate a ReferenceAttribute instead of a field one.

It's true that replacing the field attribute by a ref attr downstream in the plan will make InsertFieldExtrac gloss over the missing attribute altogether.

But InsertFieldExtract's existing mechanism already compares with the upstream output by name id (we use attribute sets), so this is already taken care of. Or should be, anyway.