clintropolis commented on code in PR #15552:
URL: https://github.com/apache/druid/pull/15552#discussion_r1425901179
##########
processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java:
##########
@@ -157,9 +157,12 @@ public int hashCode()
class LongExpr extends ConstantExpr<Long>
{
+ final ExprEval expr;
Review Comment:
nit: private?
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java:
##########
@@ -55,6 +55,9 @@ public static ExpressionType autoDetect(ExprEval eval,
ExprEval otherEval)
{
ExpressionType type = eval.type();
ExpressionType otherType = otherEval.type();
+ if (type == otherType && eval.value() != null && otherEval.value() !=
null) {
Review Comment:
why does this need more than `type == otherType`? Later on we do
```
type = eval.value() != null ? type : otherType;
otherType = otherEval.value() != null ? otherType : type;
```
so if the value is null then it just takes on the other type, so i think
being null or not here doesn't matter, because if the value is null and the
same type then it is fine
##########
processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java:
##########
@@ -464,15 +473,18 @@ public String toString()
class ComplexExpr extends ConstantExpr<Object>
{
+ final ExprEval expr;
Review Comment:
I'm not sure how often `Expr` are used in equals/hashcode, my gut says not
often since the thing that makes them also usually has the original string
which was parsed into `Expr` and is likely cheaper to just use the string
--
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]
