ESQL: Fix attribute set equals #118823
ESQL: Fix attribute set equals #118823
Conversation
|
Hi @alex-spies, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| @@ -2190,6 +2191,36 @@ public void testLookupJoinUnknownField() { | |||
| assertThat(e.getMessage(), containsString(errorMessage3 + "right side of join")); | |||
| } | |||
|
|
|||
| public void testMultipleLookupJoinsGiveDifferentAttributes() { | |||
There was a problem hiding this comment.
@craigtaverner , we were chatting about attributes obtained from different LOOKUP JOINs. This test demonstrates that they do the right thing!
| if (obj instanceof AttributeSet as) { | ||
| obj = as.delegate; | ||
| } | ||
|
|
||
| return delegate.equals(obj); |
There was a problem hiding this comment.
Do we expect it to ever be equal if object is not AttributeSet? If not, following could avoid reassigning the parameter
| if (obj instanceof AttributeSet as) { | |
| obj = as.delegate; | |
| } | |
| return delegate.equals(obj); | |
| return obj instanceof AttributeSet as && Objects.equals(as.delegate, this.delegate); |
There was a problem hiding this comment.
Yeah, I think your approach would be stricter and I'd write it like this if I was writing this from scratch.
However, that'd potentially rule previously equal sets to not be equal anymore, even though that probably never applied in reality. But we'd have to go and check. And this'd be inconsistent with AttributeMap, which also defers to delegate.equals even for instances of different classes.
|
The serverless checks fail for a single, completely unrelated test failure (already tracked in https://github.com/elastic/elasticsearch-serverless/issues/3313), so I believe this is fine to merge regardless. |
💚 Backport successful
|
Also add a test that uses this, for lookup join field attribute ids.
Also add a test that uses this, for lookup join field attribute ids.
AttributeSet.equalswas made so that one set was never ever equal to another; because we used the underlyingAttributeMapand delegated toAttributeMap.equals, which requires equal classes first and foremost.Fix that, following the pattern from
AttributeMap.equals.And, while at it, let's add a test that uses this and checks that name ids from different
LOOKUP JOINcommands do not overlap.