You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/05/23 23:08:13 UTC

[GitHub] [pinot] Jackie-Jiang opened a new pull request, #10799: [Multi-stage] Support null in aggregate and filter

Jackie-Jiang opened a new pull request, #10799:
URL: https://github.com/apache/pinot/pull/10799

   (no comment)


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] somandal commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204868921


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   Thanks for getting back on this! So to make sure I understand correctly, when the data is sent from leaf to intermediate, the null blocks will be correctly set to null in the `List<Object[]>`? Thus making a simple null check like this good enough?



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] somandal commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204834820


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   Just curious, today the leaf node leverages the nullValueBitmap, but this is not available in the intermediate stages.
   
   Example:
   ```
       if (_nullHandlingEnabled) {
         // TODO: avoid the null bitmap check when it is null or empty for better performance.
         RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
         if (nullBitmap == null) {
           nullBitmap = new RoaringBitmap();
         }
         aggregateNullHandlingEnabled(length, aggregationResultHolder, blockValSet, nullBitmap);
         return;
       }
   ```
   
   Now we usually split the LogicalAggregate into an intermediate stage (which may or may not run on leaf) and a final stage (which probably won't run on leaf as it needs to do the final aggregations). Does this mean that the nullValueBitmap won't be leveraged if this intermediate stage doesn't run in the leaf? will that affect query results? 



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204887917


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   Correct. If null handling is enabled, `null` should be properly set into the `Object[]`, and the null bitmap is already dropped



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] somandal commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204908782


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   Got it, thanks for explaining in detail! Checking out that part of the code



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] Jackie-Jiang merged pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10799:
URL: https://github.com/apache/pinot/pull/10799


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] walterddr commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204900676


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   > when the data is sent from leaf to intermediate, the null values will be correctly set to null in the `List<Object[]>`? Thus making a simple null check like this good enough?
   
   this statement is not entirely true. 
   - when sent, the data is still Ser/De into bitmap (see `DataBlock`)
   - when used, it is pulled from `TransferableBlock` which has 2 member variable format: (1) `DataBlock _dataBlock` format and (2) `List<Object[]> _container`
   
   it is when the lazy eval of the _container that puts the null in the right place
   



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] walterddr commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204900676


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   > when the data is sent from leaf to intermediate, the null values will be correctly set to null in the `List<Object[]>`? Thus making a simple null check like this good enough?
   
   this statement is not entirely true. 
   - when sent, the data is still Ser/De into bitmap (see `DataBlock`)
   - when used, it is pulled from `TransferableBlock` which has 2 member variable format: (1) `DataBlock _dataBlock` format and (2) `List<Object[]> _container`
   - it is when the lazy eval of the `_container` getter that puts the null in the right place
   



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] codecov-commenter commented on pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10799:
URL: https://github.com/apache/pinot/pull/10799#issuecomment-1560266739

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#10799](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c3e69c2) into [master](https://app.codecov.io/gh/apache/pinot/commit/d6862727c2352c939ea55e14f834e73d8b39cb95?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d686272) will **decrease** coverage by `54.82%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #10799       +/-   ##
   =============================================
   - Coverage     68.50%   13.69%   -54.82%     
   + Complexity     5649      439     -5210     
   =============================================
     Files          2159     2105       -54     
     Lines        116009   113550     -2459     
     Branches      17560    17263      -297     
   =============================================
   - Hits          79474    15548    -63926     
   - Misses        30907    96731    +65824     
   + Partials       5628     1271     -4357     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `13.69% <0.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...inot/query/runtime/operator/AggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9BZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
   | [...uery/runtime/operator/WindowAggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9XaW5kb3dBZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-95.29%)` | :arrow_down: |
   | [...query/runtime/operator/operands/FilterOperand.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9vcGVyYW5kcy9GaWx0ZXJPcGVyYW5kLmphdmE=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
   | [...ry/runtime/operator/operands/TransformOperand.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9vcGVyYW5kcy9UcmFuc2Zvcm1PcGVyYW5kLmphdmE=) | `0.00% <0.00%> (-73.08%)` | :arrow_down: |
   | [...query/runtime/operator/utils/AggregationUtils.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci91dGlscy9BZ2dyZWdhdGlvblV0aWxzLmphdmE=) | `0.00% <0.00%> (-96.56%)` | :arrow_down: |
   | [...ry/runtime/operator/utils/FunctionInvokeUtils.java](https://app.codecov.io/gh/apache/pinot/pull/10799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci91dGlscy9GdW5jdGlvbkludm9rZVV0aWxzLmphdmE=) | `0.00% <ø> (-100.00%)` | :arrow_down: |
   
   ... and [1663 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10799/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] walterddr commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204839223


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   try to piece out what this comment is about, here is my understanding:
   
   - intermediate stage is run on `List<Object[]>` so it can naturally represent null with the object array
   - what you mentioned with nullValueBitmap is being used in 2 places
       - in leaf operators that will read it out when `enableNullHandling` flag is turned on
       - in data table ser/de --> this also means when intermediate stage received the payload, it will deserialized it into `List<Object[]>` with the null put right into places. 
   
   please let me know if this answers your question



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

[GitHub] [pinot] somandal commented on a diff in pull request #10799: [Multi-stage] Support null in aggregate and filter

Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10799:
URL: https://github.com/apache/pinot/pull/10799#discussion_r1204868921


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/AggregationUtils.java:
##########
@@ -54,54 +54,92 @@ public static Key extractEmptyKey() {
     return new Key(new Object[0]);
   }
 
-  private static Object mergeSum(Object left, Object right) {
-    return ((Number) left).doubleValue() + ((Number) right).doubleValue();
+  // TODO: Use the correct type for SUM/MIN/MAX instead of always using double
+
+  @Nullable
+  private static Object mergeSum(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return ((Number) agg).doubleValue() + ((Number) value).doubleValue();
   }
 
-  private static Object mergeMin(Object left, Object right) {
-    return Math.min(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMin(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {
+      return value;
+    }
+    if (value == null) {
+      return agg;
+    }
+    return Math.min(((Number) agg).doubleValue(), ((Number) value).doubleValue());
   }
 
-  private static Object mergeMax(Object left, Object right) {
-    return Math.max(((Number) left).doubleValue(), ((Number) right).doubleValue());
+  @Nullable
+  private static Object mergeMax(@Nullable Object agg, @Nullable Object value) {
+    if (agg == null) {

Review Comment:
   Thanks for getting back on this! So to make sure I understand correctly, when the data is sent from leaf to intermediate, the null values will be correctly set to null in the `List<Object[]>`? Thus making a simple null check like this good enough?



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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