SQL: [Tests] Fix replaceChildren of Pivot#49004
SQL: [Tests] Fix replaceChildren of Pivot#49004matriv wants to merge 1 commit intoelastic:masterfrom
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
|
Pinging @elastic/es-search (:Search/SQL) |
astefan
left a comment
There was a problem hiding this comment.
LGTM, but I would also add a comment related to why this breaks now (and wasn't breaking since Pivot was introduced).
costin
left a comment
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.
| } | ||
|
|
||
| @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?
| 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