ESQL: Fix LOOKUP attribute shadowing#109807
ESQL: Fix LOOKUP attribute shadowing#109807elasticsearchmachine merged 22 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @alex-spies, I've created a changelog YAML for you. |
| ; | ||
|
|
||
|
|
||
| shadowing |
There was a problem hiding this comment.
Added analogous csv tests to LOOKUP, ENRICH, DISSECT, GROK and EVAL, since they should handle attribute shadowing identically.
| /** | ||
| * References to the input fields to match against the {@link #localRelation}. | ||
| */ | ||
| private final List<NamedExpression> matchFields; |
There was a problem hiding this comment.
These are always attributes (unresolved or resolved); the full generality of NamedExpression (including Alias etc.) makes reasoning about the match fields later down the road needlessly hard.
| List<Attribute> fieldsAddedFromRight = removeCollisionsWithMatchFields(rightOutput, matchFieldSet, matchFieldNames); | ||
| yield mergeOutputAttributes(makeNullable(makeReference(fieldsAddedFromRight)), leftOutput); |
There was a problem hiding this comment.
Using mergeOutputAttributes correctly here is the crux of this PR.
|
I'm a little conflicted on whether some logical plans should have unit tests. Arguably, we do catch inconsistent logical plan outputs either via the dependency checker or in csv tests; on the other hand, computing the output esp. for |
| } | ||
| } | ||
| return results; | ||
| return attributes.stream().filter(attr -> matchFields.contains(attr) || matchFieldNames.contains(attr.name()) == false).toList(); |
There was a problem hiding this comment.
We tend to avoid using streams, except tests, as it's an unnecessary runtime complication.
| super(Source.readFrom(in), in.readPhysicalPlanNode()); | ||
| this.joinData = new LocalSourceExec(in); | ||
| this.matchFields = in.readNamedWriteableCollectionAsList(NamedExpression.class); | ||
| this.matchFields = (List<Attribute>) (List) in.readNamedWriteableCollectionAsList(NamedExpression.class); |
There was a problem hiding this comment.
Since we haven't cut a release of with this and we don't allow these constructs in serverless yet I think you can change this to in.readNamedWriteableCollectionAsList(Attribute.class). There isn't any code that'll ever send anything that isn't an Attribute.
Also, if these were always Attribute subclasses in practice then this'd be safe here too - it's just on the read side.
Fix #109392
This makes attribute shadowing of LOOKUP consistent with ENRICH, DISSECT/GROK and EVAL.