clintropolis commented on a change in pull request #11170:
URL: https://github.com/apache/druid/pull/11170#discussion_r634014087
##########
File path:
processing/src/main/java/org/apache/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java
##########
@@ -97,26 +104,30 @@ public long getLong()
@Override
public ExprEval getObject()
{
- // things can still call this even when underlying selector is null (e.g.
ExpressionColumnValueSelector#isNull)
+ boolean cached;
+ boolean currentInputIsNull = false;
if (selector.isNull()) {
- return ExprEval.ofLong(null);
+ cached = cachedIsNull;
+ currentInputIsNull = true;
+ } else {
+ cached = selector.getLong() == lastInput && lastOutput != null;
Review comment:
This seems a bit over complicated to me, and isn't fully taking of
advantage of the cached null expression evaluation you've added. What about if
we instead did something like what you've got so far, where we are always
saving a special cached value for null, but maybe annotated slightly
differently to indicate its behavior:
```java
@MonotonicNonNull
private ExprEval nullOutput;
```
and then this block looks like:
```java
if (selector.isNull()) {
if (nullOutput == null) {
bindings.set(null);
nullOutput = expression.eval(bindings);
}
return nullOutput;
}
```
and none of the logic after the `selector.isNull()` check has to change at
all because there isn't really a need to go through the remaining logic if the
underlying selector is null. I tested this and the test you've added still
passes, and I think it would make the logic a bit clearer to follow. What do
you think?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
