SQL: [Tests] Fix replaceChildren of Pivot #49004
Conversation
For the special case that the `qualifier` of an attribute is set to null, when the attribute is still an `UnresolvedAttribute` there was a call to `dataType()` which throws an exception. To avoid that the method `withQualifier()` is overriden for `UnresolvedAttribute`. Fixes: elastic#48900
matriv
added
>test-failure
|
Pinging @elastic/es-search (:Search/SQL) |
There was a problem hiding this comment.
I don't like this approach with propagates the hack of trying to bend equality when things are not really equal.
Modifying an unresolved attribute should throw an exception period. Acting as if nothing happened it's a bug.
A better fix would be to change the nodes test to avoid having unresolved attributes. An even better one would be to simply copy the node without doing any transformation on it (hence why it fails) - if there are any changes then these need to be addressed somewhere else.
Lastly, I've mentioned several times in the past please stop formatting the whole file, only the lines that are actually touched.
| } | ||
| return e; | ||
| } | ||
|
|
||
| @Override | ||
| protected NodeInfo<Pivot> info() { |
There was a problem hiding this comment.
I haven't reviewed the previous PR so there's not much to do at this point.
I find the transformation inside info an ugly hack - from a simple copy it became transformer that overrides qualifiers.
What if the relationship has qualifiers in it?
| @@ -99,6 +99,11 @@ public String unresolvedMessage() { | |||
| return unresolvedMsg; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
A simple fix yet incorrect since an unresolved attribute should be first resolved before being manipulated.
It's way none of the with methods are implemented - touching an unresolved attribute is incorrect period.
|
Superseded by #49693 where the refactoring addressed and fixed the issue in a better way. |
For the special case that the
qualifierof an attribute is set tonull, when the attribute is still an
UnresolvedAttributethere was acall to
dataType()which throws an exception. To avoid that the methodwithQualifier()is overriden forUnresolvedAttribute.Fixes: #48900