kgyrtkirk commented on code in PR #15365:
URL: https://github.com/apache/druid/pull/15365#discussion_r1400303376
##########
processing/src/main/java/org/apache/druid/query/operator/window/WindowFramedAggregateProcessor.java:
##########
@@ -96,4 +96,32 @@ public String toString()
", aggregations=" + Arrays.toString(aggregations) +
'}';
}
+
+ @Override
+ public int hashCode()
+ {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + Arrays.hashCode(aggregations);
+ result = prime * result + Objects.hash(frame);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj)
+ {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null) {
+ return false;
+ }
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+ WindowFramedAggregateProcessor other = (WindowFramedAggregateProcessor)
obj;
+ return Arrays.equals(aggregations, other.aggregations) &&
Objects.equals(frame, other.frame);
+ }
Review Comment:
There are tests which use these - its a bit hard to take a look at a list of
operators without clear context about what the named fields will
contain...without context that makes it pretty hard to follow - and may even
need a complicated detour [like in this
PR](https://github.com/apache/druid/pull/14007#discussion_r1363362711)
I understand the intention but I don't think these equiv things will be that
helpfull in the longrun:
I wonder if
`select col as u,1 as v from t`
should be considered equivalent to
`select col as v,1 as u from t`
?
probably...
what if its being used as a subquery:
`select substring(u,v) from T`
then I guess they are clearly not equiv
w.r.t. to window processors; is it the same:
```
rank() as u,
row_number() as v
```
and
```
rank() as v,
row_number() as u
```
?
I guess right now that would return `true` - but I believe that they
equivalence depends on the outer context not just on the expression.
To get real equivalence level comparision the whole tree should be either
rewritten or compared via some stuff (observer?) which could register and could
keep track equiv pieces; while retaining the concept of left and right - but
that's now how these `validateEquivalent` calls are designed
I don't know what benefits are we getting from taking the above risks
`validateEquivalent` may hide; I think it it might be better to just remove
these `validateEquivalent` things and rather go with `equals` and `hashCode` -
there are pros and cons; but they also provide that things can be put inside a
`Set` correctly...
so far the only pro is that they don't need to set the `outputColumn`
properly...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
