gianm commented on code in PR #16084:
URL: https://github.com/apache/druid/pull/16084#discussion_r1517812170
##########
processing/src/test/java/org/apache/druid/math/expr/ConstantExprTest.java:
##########
@@ -34,5 +35,6 @@ public void testLongSingleThreadedExpr()
assertNotSame(expr.eval(null), expr.eval(null));
Expr singleExpr = Expr.singleThreaded(expr);
assertSame(singleExpr.eval(null), singleExpr.eval(null));
+ assertEquals(expr.toString(), singleExpr.toString());
Review Comment:
We should have a test for `stringify` too as that method is expected to be a
legit API. Would also be nice to have tests for other types of constant expr,
and tests that the `stringify` ends up as specific strings (rather than just
that the orig and single-threaded ones match).
##########
processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java:
##########
@@ -145,6 +152,18 @@ protected ExprEval<T> realEval()
return eval;
}
+ @Override
+ public String stringify()
+ {
+ return origExpr.stringify();
Review Comment:
Is `stringify` in all possible `origExpr` thread-safe? This approach would
require that. Would be good to have a comment here mentioning that too.
Alternatively is it possible to implement this using the ExprEval? Like
`eval.toExpr.stringify`? Would generate more garbage, but be simpler
structurally and then we wouldn't have to worry about any possible issues
related to retaining a ref to the original expr. Also, stringify isn't called
in hot loops so the extra garbage isn't as important.
--
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]
