[GitHub] [druid] clintropolis commented on a change in pull request #11170: Fix is null selector returning incorrect value for Long data type

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org