You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/03/22 00:59:38 UTC

[GitHub] [pinot] nizarhejazi opened a new pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

nizarhejazi opened a new pull request #8382:
URL: https://github.com/apache/pinot/pull/8382


   ## Description
   PR to fix github issue: https://github.com/apache/pinot/issues/8374
   
   Add an annotation for DateTime functions that cannot take null, and just return null when the function is annotated and the input value is null.
   
   **<code>release-notes</code>**
   ## Release Notes
   Prevent datetime functions that cannot work w/ null from throwing NullPointerException. Preserve null input values so they can be marked as such in null index.
   


-- 
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] richardstartin commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r832652223



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -131,4 +133,19 @@ public Object invoke(Object[] arguments) {
           "Caught exception while invoking method: " + _method + " with arguments: " + Arrays.toString(arguments), e);
     }
   }
+
+  /**
+   * Whether method is a scalar function with nullableParameters property set to true.
+   */
+  public boolean hasNullableParameters() {
+    for (final Annotation annotation : _method.getAnnotations()) {
+      if (annotation.annotationType().equals(ScalarFunction.class)) {
+        final ScalarFunction scalarFunctionAnnotation = (ScalarFunction) annotation;
+        if (scalarFunctionAnnotation.nullableParameters()) {
+          return true;
+        }
+      }
+    }
+    return false;

Review comment:
       I think the following would be cleaner (and probably more efficient)
   ```java
   return _method.isAnnotationPresent(ScalarFunction.class) && _method.getAnnotation(ScalarFunction.class).nullableParameters();
   ```




-- 
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] nizarhejazi commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833756927



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -306,7 +306,7 @@ public static long now() {
    *           "-P6H3M"    -- parses as "-6 hours and -3 minutes"
    *           "-P-6H+3M"  -- parses as "+6 hours and -3 minutes"
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       `ago(periodString)` does not handle null because it cannot return null (since return type is primitive `long`, not wrapper `Long`). I thought the idea is to mark any function that cannot handle null because return type is primitive (cannot return null) w/ nullableParameters.
   
   Anyway, removed the annotation from all functions except fromDateTime and dateTimeConvert since both can be used as ingestion transforms with input columns that can take null value.




-- 
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] nizarhejazi commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833761654



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -124,6 +126,15 @@ public void convertTypes(Object[] arguments) {
    * {@link #convertTypes(Object[])} to convert the argument types if needed before calling this method.
    */
   public Object invoke(Object[] arguments) {
+    if (_nullableParameters) {

Review comment:
       makes sense




-- 
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 change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r832657301



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -131,4 +133,19 @@ public Object invoke(Object[] arguments) {
           "Caught exception while invoking method: " + _method + " with arguments: " + Arrays.toString(arguments), e);
     }
   }
+
+  /**
+   * Whether method is a scalar function with nullableParameters property set to true.
+   */
+  public boolean hasNullableParameters() {
+    for (final Annotation annotation : _method.getAnnotations()) {
+      if (annotation.annotationType().equals(ScalarFunction.class)) {
+        final ScalarFunction scalarFunctionAnnotation = (ScalarFunction) annotation;
+        if (scalarFunctionAnnotation.nullableParameters()) {
+          return true;
+        }
+      }
+    }
+    return false;

Review comment:
       The annotation parsing logic can be moved to `FunctionRegistry`, and the nullable info can be stored in `FunctionInfo`

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -247,7 +247,7 @@ public static Timestamp toTimestamp(long millis) {
   /**
    * Converts Timestamp to epoch millis
    */
-  @ScalarFunction

Review comment:
       Why are they annotated as nullable? The input should not be null




-- 
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 edited a comment on pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075400850


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8382](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (425611d) into [master](https://codecov.io/gh/apache/pinot/commit/2d809ffe33e75ae5d6f8ed2c16ec1442fa1d6adf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d809ff) will **decrease** coverage by `41.12%`.
   > The diff coverage is `47.05%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8382       +/-   ##
   =============================================
   - Coverage     69.75%   28.63%   -41.13%     
   =============================================
     Files          1653     1642       -11     
     Lines         86615    86133      -482     
     Branches      13080    13008       -72     
   =============================================
   - Hits          60420    24662    -35758     
   - Misses        21966    59187    +37221     
   + Partials       4229     2284     -1945     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `28.63% <47.05%> (+0.07%)` | :arrow_up: |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   
   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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...he/pinot/common/function/scalar/JsonFunctions.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0pzb25GdW5jdGlvbnMuamF2YQ==) | `15.15% <ø> (-72.73%)` | :arrow_down: |
   | [...gment/local/function/InbuiltFunctionEvaluator.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9JbmJ1aWx0RnVuY3Rpb25FdmFsdWF0b3IuamF2YQ==) | `0.00% <0.00%> (-88.71%)` | :arrow_down: |
   | [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `88.88% <100.00%> (-11.12%)` | :arrow_down: |
   | [...apache/pinot/common/function/FunctionRegistry.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25SZWdpc3RyeS5qYXZh) | `94.11% <100.00%> (+0.17%)` | :arrow_up: |
   | [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1157 more](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2d809ff...425611d](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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 #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075400850


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8382](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c9f729a) into [master](https://codecov.io/gh/apache/pinot/commit/2d809ffe33e75ae5d6f8ed2c16ec1442fa1d6adf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d809ff) will **decrease** coverage by `55.64%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8382       +/-   ##
   =============================================
   - Coverage     69.75%   14.11%   -55.65%     
   + Complexity     4212       81     -4131     
   =============================================
     Files          1653     1608       -45     
     Lines         86615    84745     -1870     
     Branches      13080    12881      -199     
   =============================================
   - Hits          60420    11962    -48458     
   - Misses        21966    71884    +49918     
   + Partials       4229      899     -3330     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `14.11% <0.00%> (+0.03%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../apache/pinot/common/function/FunctionInvoker.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbnZva2VyLmphdmE=) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
   | [...inot/common/function/scalar/DateTimeFunctions.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `0.00% <ø> (-95.96%)` | :arrow_down: |
   | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1296 more](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2d809ff...c9f729a](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] KKcorps commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
KKcorps commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r835083701



##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -138,6 +149,15 @@ public Object execute(Object[] values) {
         for (int i = 0; i < numArguments; i++) {
           _arguments[i] = _argumentNodes[i].execute(values);
         }
+        if (!_functionInfo.hasNullableParameters()) {

Review comment:
       @Jackie-Jiang with this logic, by default, for all of our scalar functions if an argument is null we will return null instead of throwing exception. Is this fine?




-- 
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] nizarhejazi commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833756927



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -306,7 +306,7 @@ public static long now() {
    *           "-P6H3M"    -- parses as "-6 hours and -3 minutes"
    *           "-P-6H+3M"  -- parses as "+6 hours and -3 minutes"
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       `ago(periodString)` does not handle null because it cannot return null (since return type is primitive `long`, not wrapper `Long`). Anyway, removed the annotation.




-- 
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 pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075520071


   @nizarhejazi If a method can take null parameters, it should handle all the parameters properly. In the scenario you described, the method can return null when it cannot handle the passed in null value.
   The reason why we might not want to check nulls during `invoke` is that this will bring overhead to all invocations of the function, but in most places the arguments won't be null


-- 
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] nizarhejazi edited a comment on pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi edited a comment on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075475131


   @Jackie-Jiang There is an issue w/ defining nullableParameters in the annotation that is applied at the method level (vs. at the parameter level). What if the scalar function has more than one parameter (e.g. dateTimeConvert) and the function cannot handle the case where only the main input parameter is null (the input column value), while it is perfectly valid for the other parameters to be nulls?


-- 
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] nizarhejazi commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833756927



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -306,7 +306,7 @@ public static long now() {
    *           "-P6H3M"    -- parses as "-6 hours and -3 minutes"
    *           "-P-6H+3M"  -- parses as "+6 hours and -3 minutes"
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       `ago(periodString)` does not handle null because it cannot return null (since return type is primitive `long`, not wrapper `Long`). I thought the idea is to mark any function that cannot handle null because return type is primitive (cannot return null) w/ nullableParameters.
   
   Anyway, removed the annotation.




-- 
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 change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r835542626



##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -138,6 +149,15 @@ public Object execute(Object[] values) {
         for (int i = 0; i < numArguments; i++) {
           _arguments[i] = _argumentNodes[i].execute(values);
         }
+        if (!_functionInfo.hasNullableParameters()) {

Review comment:
       Yes, and that is the expected behavior. Then the record transformer can fill the default value for this column

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -138,6 +149,15 @@ public Object execute(Object[] values) {
         for (int i = 0; i < numArguments; i++) {
           _arguments[i] = _argumentNodes[i].execute(values);
         }
+        if (!_functionInfo.hasNullableParameters()) {

Review comment:
       Yes, and that is the desired behavior. Then the record transformer can fill the default value for this column




-- 
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 change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r834590900



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -84,7 +84,7 @@ public static String jsonFormat(Object object)
   /**
    * Extract object based on Json path
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       No need to annotate this function because when input is `null`, output is also `null`. The null check on line 92 can actually be removed with the change in this PR because that will never be hit

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -103,7 +103,7 @@ public static Object jsonPath(Object object, String jsonPath) {
     return convertObjectToArray(PARSE_CONTEXT.parse(object).read(jsonPath, NO_PREDICATES));
   }
 
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static Object[] jsonPathArrayDefaultEmpty(Object object, String jsonPath) {

Review comment:
       `object` should be annotated as `Nullable`

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -127,7 +127,7 @@ public static Object jsonPath(Object object, String jsonPath) {
   /**
    * Extract from Json with path to String
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       Similarly, we don't need to annotate this function, and the null check can be removed

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -66,7 +66,7 @@ private JsonFunctions() {
   /**
    * Convert Map to Json String
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static String toJsonMapStr(Map map)

Review comment:
       Let's annotate the nullable parameter as `Nullable`, same for other annotated functions

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -183,15 +183,15 @@ public static long jsonPathLong(Object object, String jsonPath, long defaultValu
   /**
    * Extract from Json with path to Double
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static double jsonPathDouble(Object object, String jsonPath) {
     return jsonPathDouble(object, jsonPath, Double.NaN);
   }
 
   /**
    * Extract from Json with path to Double
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static double jsonPathDouble(Object object, String jsonPath, double defaultValue) {

Review comment:
       `object` should be annotated as `Nullable`

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -183,15 +183,15 @@ public static long jsonPathLong(Object object, String jsonPath, long defaultValu
   /**
    * Extract from Json with path to Double
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       Similarly, don't annotate this function

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -156,15 +156,15 @@ public static String jsonPathString(Object object, String jsonPath, String defau
   /**
    * Extract from Json with path to Long
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       Let's not annotate this function. When the input is null, we want to return null so that the actual default value defined in the schema can be filled

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
##########
@@ -156,15 +156,15 @@ public static String jsonPathString(Object object, String jsonPath, String defau
   /**
    * Extract from Json with path to Long
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static long jsonPathLong(Object object, String jsonPath) {
     return jsonPathLong(object, jsonPath, Long.MIN_VALUE);
   }
 
   /**
    * Extract from Json with path to Long
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)
   public static long jsonPathLong(Object object, String jsonPath, long defaultValue) {

Review comment:
       `object` should be annotated as `Nullable`

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -108,11 +108,13 @@ public Object evaluate(Object[] values) {
 
   private static class FunctionExecutionNode implements ExecutableNode {
     final FunctionInvoker _functionInvoker;
+    private final FunctionInfo _functionInfo;

Review comment:
       (minor) remove `private` as it is already in a private class




-- 
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] nizarhejazi commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r832713248



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -247,7 +247,7 @@ public static Timestamp toTimestamp(long millis) {
   /**
    * Converts Timestamp to epoch millis
    */
-  @ScalarFunction

Review comment:
       @Jackie-Jiang `fromTimestamp` takes Timestamp (object) that can be null but cannot handle null (returns primitive long). Btw, I see `fromTimestamp` is not being used so I removed the added annotation.
   
   I applied the same logic to other functions: `ago`, `timezoneHour`, `timezoneMinute` and `dateTimeConvert`.




-- 
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 change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833557629



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
##########
@@ -38,4 +40,8 @@ public Method getMethod() {
   public Class<?> getClazz() {
     return _clazz;
   }
+
+  public boolean getNullableParameters() {

Review comment:
       ```suggestion
     public boolean hasNullableParameters() {
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
##########
@@ -63,12 +63,13 @@ private FunctionRegistry() {
       if (scalarFunction.enabled()) {
         // Annotated function names
         String[] scalarFunctionNames = scalarFunction.names();
+        final boolean nullableParameters = scalarFunction.nullableParameters();

Review comment:
       (minor) We don't usually put `final` for local variable
   ```suggestion
           boolean nullableParameters = scalarFunction.nullableParameters();
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -306,7 +306,7 @@ public static long now() {
    *           "-P6H3M"    -- parses as "-6 hours and -3 minutes"
    *           "-P-6H+3M"  -- parses as "+6 hours and -3 minutes"
    */
-  @ScalarFunction
+  @ScalarFunction(nullableParameters = true)

Review comment:
       They should not be annotated as `nullableParameters` because `null` argument is not handled. For any function that should return `null` when any input is `null`, we should not annotate them to avoid the overhead of invoking them

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
##########
@@ -49,4 +49,7 @@
   // If empty, FunctionsRegistry registers the method name as function name;
   // If not empty, FunctionsRegistry only registers the function names specified here, the method name is ignored.
   String[] names() default {};
+
+  // Whether the scalar function cannot handle nulls, and can get invoked with null parameter(s).

Review comment:
       ```suggestion
     // Whether the scalar function expects null arguments, and won't return null when getting null arguments.
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -124,6 +126,15 @@ public void convertTypes(Object[] arguments) {
    * {@link #convertTypes(Object[])} to convert the argument types if needed before calling this method.
    */
   public Object invoke(Object[] arguments) {
+    if (_nullableParameters) {

Review comment:
       Most of the caller for this method won't pass in null arguments. To avoid the overhead of this null check, we may add a method `hasNullParameters()` and the caller can decide how to handle it. The only place where we may pass `null` arguments is `InbuildFunctionEvaluator.FunctionExecutionNode.execute(GenericRow row)`




-- 
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 edited a comment on pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075400850


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8382](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f06f02) into [master](https://codecov.io/gh/apache/pinot/commit/2d809ffe33e75ae5d6f8ed2c16ec1442fa1d6adf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d809ff) will **decrease** coverage by `41.15%`.
   > The diff coverage is `14.42%`.
   
   > :exclamation: Current head 6f06f02 differs from pull request most recent head 65ec971. Consider uploading reports for the commit 65ec971 to get more accurate results
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8382       +/-   ##
   =============================================
   - Coverage     69.75%   28.60%   -41.16%     
   =============================================
     Files          1653     1642       -11     
     Lines         86615    86109      -506     
     Branches      13080    13004       -76     
   =============================================
   - Hits          60420    24628    -35792     
   - Misses        21966    59202    +37236     
   + Partials       4229     2279     -1950     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `28.60% <14.42%> (+0.04%)` | :arrow_up: |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   
   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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (-80.88%)` | :arrow_down: |
   | [...inot/common/function/scalar/DateTimeFunctions.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `4.04% <ø> (-91.92%)` | :arrow_down: |
   | [...ransform/function/IdentifierTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSWRlbnRpZmllclRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `47.72% <0.00%> (-24.37%)` | :arrow_down: |
   | [...transform/function/IsNotNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOb3ROdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...or/transform/function/IsNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [.../pinot/core/query/reduce/ResultReducerFactory.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVzdWx0UmVkdWNlckZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-3.58%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/util/GapfillUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dhcGZpbGxVdGlscy5qYXZh) | `6.17% <0.00%> (-68.02%)` | :arrow_down: |
   | [...n/batch/standalone/SegmentGenerationJobRunner.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vcGlub3QtYmF0Y2gtaW5nZXN0aW9uLXN0YW5kYWxvbmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbmdlc3Rpb24vYmF0Y2gvc3RhbmRhbG9uZS9TZWdtZW50R2VuZXJhdGlvbkpvYlJ1bm5lci5qYXZh) | `0.00% <0.00%> (-63.27%)` | :arrow_down: |
   | [...gment/local/function/InbuiltFunctionEvaluator.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9JbmJ1aWx0RnVuY3Rpb25FdmFsdWF0b3IuamF2YQ==) | `0.00% <0.00%> (-88.71%)` | :arrow_down: |
   | [.../apache/pinot/common/function/FunctionInvoker.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbnZva2VyLmphdmE=) | `64.00% <50.00%> (-26.91%)` | :arrow_down: |
   | ... and [1161 more](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2d809ff...65ec971](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] richardstartin commented on a change in pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r832654127



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -131,4 +133,19 @@ public Object invoke(Object[] arguments) {
           "Caught exception while invoking method: " + _method + " with arguments: " + Arrays.toString(arguments), e);
     }
   }
+
+  /**
+   * Whether method is a scalar function with nullableParameters property set to true.
+   */
+  public boolean hasNullableParameters() {
+    for (final Annotation annotation : _method.getAnnotations()) {
+      if (annotation.annotationType().equals(ScalarFunction.class)) {
+        final ScalarFunction scalarFunctionAnnotation = (ScalarFunction) annotation;
+        if (scalarFunctionAnnotation.nullableParameters()) {
+          return true;
+        }
+      }
+    }
+    return false;

Review comment:
       or even replace `isAnnotationPresent` with a null check on the result of `getAnnotation` and if not null check if there are nullable parameters or not if you want to speed it up.




-- 
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] nizarhejazi commented on pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
nizarhejazi commented on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075475131


   @Jackie-Jiang There is an issue w/ defining nullableParameters in the annotation that is applied at the method level (vs. at the parameter level). What if the scalar function has more than one parameter (e.g. dateTimeConvert) and the function cannot handle the case where only the main input parameter is null (the input column value), while it is perfectly valid for the other parameters to be nulls?


-- 
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 edited a comment on pull request #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#issuecomment-1075400850


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8382](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (425611d) into [master](https://codecov.io/gh/apache/pinot/commit/2d809ffe33e75ae5d6f8ed2c16ec1442fa1d6adf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d809ff) will **decrease** coverage by `41.12%`.
   > The diff coverage is `47.05%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8382       +/-   ##
   =============================================
   - Coverage     69.75%   28.63%   -41.13%     
   =============================================
     Files          1653     1642       -11     
     Lines         86615    86133      -482     
     Branches      13080    13008       -72     
   =============================================
   - Hits          60420    24662    -35758     
   - Misses        21966    59187    +37221     
   + Partials       4229     2284     -1945     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `28.63% <47.05%> (+0.07%)` | :arrow_up: |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   
   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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...he/pinot/common/function/scalar/JsonFunctions.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0pzb25GdW5jdGlvbnMuamF2YQ==) | `15.15% <ø> (-72.73%)` | :arrow_down: |
   | [...gment/local/function/InbuiltFunctionEvaluator.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9JbmJ1aWx0RnVuY3Rpb25FdmFsdWF0b3IuamF2YQ==) | `0.00% <0.00%> (-88.71%)` | :arrow_down: |
   | [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `88.88% <100.00%> (-11.12%)` | :arrow_down: |
   | [...apache/pinot/common/function/FunctionRegistry.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25SZWdpc3RyeS5qYXZh) | `94.11% <100.00%> (+0.17%)` | :arrow_up: |
   | [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1157 more](https://codecov.io/gh/apache/pinot/pull/8382/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2d809ff...425611d](https://codecov.io/gh/apache/pinot/pull/8382?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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 #8382: builtin datetime functions to avoid throwing NullPointerException when input value is null

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #8382:
URL: https://github.com/apache/pinot/pull/8382


   


-- 
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