kgyrtkirk commented on PR #15694: URL: https://github.com/apache/druid/pull/15694#issuecomment-1895612070
The reason I've stuck to this approach beause @gianm have mentioned that this approach might be usefull in the future around [at the end of this comment](https://github.com/apache/druid/pull/15552#issuecomment-1879849582). Making an immutable `ExprEval` for this would be also a valid approach for sure - by creating one which doesn't rely on those fields by pre-computing all values and just returning it <details> <summary>partial code</summary> ```java public ExprEval<T> makeImmutableExprEval(ExprEval e) { int intVal = e.asInt(); long longVal = e.asLong(); String stringVal = e.asString(); return new ExprEval<T>() { @Override public int asInt() { return intVal; } @Override public long asLong() { return longVal; } @Override public String asString() { return asString(); } //[...] } } ``` </details> Another alternate approach could be to change the way caching works in `ExprEval` to not hold 2 fields; instead just one : * something like `Optional<Supplier<String>> stringValue` (can be a custom class as well - but I guess this type tells the story) - so that the field is either seen as assigned ; or unassigned (and could possibly be filled in by multiple threads - which I guess is not an issue here). * similarily for other cached stuff in `StringExprEval` -- 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]
