You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/11 00:36:21 UTC

[GitHub] [druid] somu-imply opened a new pull request #11904: IMPLY-4344: Adding safe divide function along with testcases and docu…

somu-imply opened a new pull request #11904:
URL: https://github.com/apache/druid/pull/11904


   …mentation updates
   
   <!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
   
   <!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
   
   Fixes #XXXX.
   
   <!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
   
   <!-- If you are a committer, follow the PR action item checklist for committers:
   https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
   
   ### Description
   
   <!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
   
   <!-- Describe your patch: what did you change in code? How did you fix the problem? -->
   
   <!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
   
   #### Fixed the bug ...
   #### Renamed the class ...
   #### Added a forbidden-apis entry ...
   
   <!--
   In each section, please describe design decisions made, including:
    - Choice of algorithms
    - Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
    - Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
    - Method organization and design (how the logic is split between methods, parameters and return types)
    - Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
   -->
   
   
   <!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
   
   <!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
   
   <!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
   
   <hr>
   
   ##### Key changed/added classes in this PR
    * `MyFoo`
    * `OurBar`
    * `TheirBaz`
   
   <hr>
   
   <!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
   
   This PR has:
   - [ ] been self-reviewed.
      - [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
   - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [ ] been tested in a test Druid cluster.
   


-- 
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@druid.apache.org

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



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

[GitHub] [druid] FrankChen021 commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r747151235



##########
File path: docs/misc/math-expr.md
##########
@@ -154,6 +154,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |remainder|remainder(x, y) returns the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) returns value that is closest in value to x and is equal to a mathematical integer|
 |round|round(x, y) returns the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is `NaN`, x returns 0. If x is infinity, x will be converted to the nearest finite double. |
+|safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0, returns 0 otherwise|

Review comment:
       it's better to put this new function along with the 'div' function above.




-- 
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@druid.apache.org

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



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

[GitHub] [druid] clintropolis commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r747195938



##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,46 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      return ExpressionTypeConversion.integerMathFunction(
+          args.get(0).getOutputType(inspector),
+          args.get(1).getOutputType(inspector)
+      );
+    }
+
+    @Override
+    protected ExprEval eval(final long x, final long y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }
+      return ExprEval.of(x / y);
+    }
+
+    @Override
+    protected ExprEval eval(final double x, final double y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }

Review comment:
       same suggestion (and also should preserve double typing)
   ```suggestion
         if (y == 0 && x != 0) {
           return ExprEval.ofDouble(NullHandling.defaultDoubleValue());
         }
   ```

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java
##########
@@ -0,0 +1,34 @@
+package org.apache.druid.sql.calcite.expression.builtin;

Review comment:
       missing apache license header, can copy from any other file

##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -732,6 +732,16 @@ public void testSizeFormatInvalidArgumentSize()
           .eval(bindings);
   }
 
+  @Test
+  public void testSafeDivide()
+  {
+    // happy path maths
+    assertExpr("safe_divide(3, 1)", 3L);
+    assertExpr("safe_divide(4.5, 2)", 2.25);
+    assertExpr("safe_divide(3, 0)", 0L);
+    assertExpr("safe_divide(3.7, 0.0)", 0L);

Review comment:
       if other suggestion is applied:
   
   ```suggestion
       assertExpr("safe_divide(3, 0)", NullHandling.defaultLongValue());
       assertExpr("safe_divide(3.7, 0.0)", NullHandling.defaultDoubleValue());
   ```

##########
File path: docs/misc/math-expr.md
##########
@@ -154,6 +154,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |remainder|remainder(x, y) returns the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) returns value that is closest in value to x and is equal to a mathematical integer|
 |round|round(x, y) returns the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is `NaN`, x returns 0. If x is infinity, x will be converted to the nearest finite double. |
+|safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0, returns 0 otherwise|

Review comment:
       they appear to be alphabetically sorted right now, maybe we should consider leaving it where it is (though I don't have a strong opinion)
   
   It is worth documenting the null handling behavior (especially if we change it to return null in SQL compatible null handling mode)

##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,46 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      return ExpressionTypeConversion.integerMathFunction(
+          args.get(0).getOutputType(inspector),
+          args.get(1).getOutputType(inspector)
+      );
+    }
+
+    @Override
+    protected ExprEval eval(final long x, final long y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }

Review comment:
       hmm, looking at the behavior of another `SAFE_DIVIDE` I could find in other databases (https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#safe_divide), I think we should consider returning `null` here if `NullHandling.replaceWithDefault()` is true
   
   ```suggestion
         if (y == 0 && x != 0) {
           return ExprEval.ofLong(NullHandling.defaultLongValue());
         }
   ```

##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,46 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      return ExpressionTypeConversion.integerMathFunction(

Review comment:
       I think this should actually use `ExpressionTypeConversion.function` to behave more like `/` than like `div`. 




-- 
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@druid.apache.org

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



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

[GitHub] [druid] somu-imply commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
somu-imply commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r748567496



##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,46 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      return ExpressionTypeConversion.integerMathFunction(
+          args.get(0).getOutputType(inspector),
+          args.get(1).getOutputType(inspector)
+      );
+    }
+
+    @Override
+    protected ExprEval eval(final long x, final long y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }

Review comment:
       Change made. Added a case and testcases where x or y is NaN or +- Infinity




-- 
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@druid.apache.org

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



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

[GitHub] [druid] clintropolis commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r748659135



##########
File path: docs/misc/math-expr.md
##########
@@ -154,6 +154,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |remainder|remainder(x, y) returns the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) returns value that is closest in value to x and is equal to a mathematical integer|
 |round|round(x, y) returns the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is `NaN`, x returns 0. If x is infinity, x will be converted to the nearest finite double. |
+|safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0. In case y is 0 it returns null or default values (if returning default values are enabled). |

Review comment:
       nit: to match other docs that describe difference between default and sql compatible null handling mode
   ```suggestion
   |safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0. In case y is 0 it returns 0, or `null` if `druid.generic.useDefaultValueForNull=false`. |
   ```
   
   Also, since this file doesn't backtick the expression names, `safe_divide` needs added to the spelling exceptions file https://github.com/apache/druid/blob/master/website/.spelling#L1202

##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,53 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      ExpressionType type = ExpressionType.DOUBLE;
+      for (Expr arg : args) {
+        type = ExpressionTypeConversion.function(type, arg.getOutputType(inspector));
+      }
+      return ExpressionType.asArrayType(type);

Review comment:
       this shouldn't be array typed
   ```suggestion
         return ExpressionTypeConversion.function(
             args.get(0).getOutputType(inspector),
             args.get(1).getOutputType(inspector)
         );
   ```
   
   Could you also add some tests with a mix of longs and doubles to https://github.com/apache/druid/blob/master/core/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java#L163?

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.math.expr.Function;
+import org.apache.druid.sql.calcite.expression.DirectOperatorConversion;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+
+public class SafeDivideOperatorConversion extends DirectOperatorConversion
+{
+  private static final SqlFunction SQL_FUNCTION = OperatorConversions
+      .operatorBuilder(StringUtils.toUpperCase(Function.SafeDivide.NAME))
+      .operandTypeChecker(OperandTypes.ANY_NUMERIC)
+      .returnTypeInference(ReturnTypes.QUOTIENT_NULLABLE)
+      .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION)

Review comment:
       nit: this could also probably be `SqlFunctionCategory.NUMERIC`, though tbh i'm not sure if it matters that much?

##########
File path: docs/querying/sql.md
##########
@@ -414,9 +414,11 @@ to FLOAT. At runtime, Druid will widen 32-bit floats to 64-bit for most expressi
 |`BITWISE_SHIFT_LEFT(expr1, expr2)`|Returns the result of `expr1 << expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
 |`BITWISE_SHIFT_RIGHT(expr1, expr2)`|Returns the result of `expr1 >> expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
 |`BITWISE_XOR(expr1, expr2)`|Returns the result of `expr1 ^ expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
+|`DIV(x,y)`|Returns the result of integer division of x by y |
 |`HUMAN_READABLE_BINARY_BYTE_FORMAT(value[, precision])`| Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. For example, HUMAN_READABLE_BINARY_BYTE_FORMAT(1048576) returns `1.00 MiB`. `precision` must be in the range of [0,3] (default: 2). |
 |`HUMAN_READABLE_DECIMAL_BYTE_FORMAT(value[, precision])`| Format a number in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. HUMAN_READABLE_DECIMAL_BYTE_FORMAT(1048576) returns `1.04 MB`. `precision` must be in the range of [0,3] (default: 2). `precision` must be in the range of [0,3] (default: 2). |
 |`HUMAN_READABLE_DECIMAL_FORMAT(value[, precision])`| Format a number in human-readable SI format. For example, HUMAN_READABLE_DECIMAL_FORMAT(1048576) returns `1.04 M`. `precision` must be in the range of [0,3] (default: 2). |
+|`SAFE_DIVIDE(x,y)`|Returns the division of x by y guarded on division by 0 |

Review comment:
       ```suggestion
   |`SAFE_DIVIDE(x,y)`|Returns the division of x by y guarded on division by 0. In case y is 0 it returns 0, or `null` if `druid.generic.useDefaultValueForNull=false` |
   ```




-- 
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@druid.apache.org

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



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

[GitHub] [druid] somu-imply commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
somu-imply commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r748567764



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java
##########
@@ -0,0 +1,34 @@
+package org.apache.druid.sql.calcite.expression.builtin;

Review comment:
       License headed added, alphabetical order maintained, null handling info added




-- 
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@druid.apache.org

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



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

[GitHub] [druid] somu-imply commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
somu-imply commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r749820705



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.OperandTypes;
+import org.apache.calcite.sql.type.ReturnTypes;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.math.expr.Function;
+import org.apache.druid.sql.calcite.expression.DirectOperatorConversion;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+
+public class SafeDivideOperatorConversion extends DirectOperatorConversion
+{
+  private static final SqlFunction SQL_FUNCTION = OperatorConversions
+      .operatorBuilder(StringUtils.toUpperCase(Function.SafeDivide.NAME))
+      .operandTypeChecker(OperandTypes.ANY_NUMERIC)
+      .returnTypeInference(ReturnTypes.QUOTIENT_NULLABLE)
+      .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION)

Review comment:
       Left as is




-- 
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@druid.apache.org

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



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

[GitHub] [druid] FrankChen021 commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r747151612



##########
File path: docs/querying/sql.md
##########
@@ -414,9 +414,11 @@ to FLOAT. At runtime, Druid will widen 32-bit floats to 64-bit for most expressi
 |`BITWISE_SHIFT_LEFT(expr1, expr2)`|Returns the result of `expr1 << expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
 |`BITWISE_SHIFT_RIGHT(expr1, expr2)`|Returns the result of `expr1 >> expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
 |`BITWISE_XOR(expr1, expr2)`|Returns the result of `expr1 ^ expr2`. Double values will be implicitly cast to longs, use `BITWISE_CONVERT_DOUBLE_TO_LONG_BITS` to perform bitwise operations directly with doubles|
+|`DIV(x,y)`|Returns the result of integer division of x by y |
 |`HUMAN_READABLE_BINARY_BYTE_FORMAT(value[, precision])`| Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. For example, HUMAN_READABLE_BINARY_BYTE_FORMAT(1048576) returns `1.00 MiB`. `precision` must be in the range of [0,3] (default: 2). |
 |`HUMAN_READABLE_DECIMAL_BYTE_FORMAT(value[, precision])`| Format a number in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. HUMAN_READABLE_DECIMAL_BYTE_FORMAT(1048576) returns `1.04 MB`. `precision` must be in the range of [0,3] (default: 2). `precision` must be in the range of [0,3] (default: 2). |
 |`HUMAN_READABLE_DECIMAL_FORMAT(value[, precision])`| Format a number in human-readable SI format. For example, HUMAN_READABLE_DECIMAL_FORMAT(1048576) returns `1.04 M`. `precision` must be in the range of [0,3] (default: 2). |
+|`SAFE_DIVIDE(x,y)`|Returns the division of x by y guarded on division by 0 |

Review comment:
       It's better to put it along with the 'DIV' function above.

##########
File path: docs/misc/math-expr.md
##########
@@ -154,6 +154,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |remainder|remainder(x, y) returns the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) returns value that is closest in value to x and is equal to a mathematical integer|
 |round|round(x, y) returns the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is `NaN`, x returns 0. If x is infinity, x will be converted to the nearest finite double. |
+|safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0, returns 0 otherwise|

Review comment:
       What if either x or y is 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@druid.apache.org

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



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

[GitHub] [druid] suneet-s commented on pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11904:
URL: https://github.com/apache/druid/pull/11904#issuecomment-965921897


   Added label `Release Notes` because this is a new function that users may want to take advantage of.


-- 
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@druid.apache.org

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



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

[GitHub] [druid] somu-imply commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
somu-imply commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r749820435



##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,53 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      ExpressionType type = ExpressionType.DOUBLE;
+      for (Expr arg : args) {
+        type = ExpressionTypeConversion.function(type, arg.getOutputType(inspector));
+      }
+      return ExpressionType.asArrayType(type);

Review comment:
       Done

##########
File path: docs/misc/math-expr.md
##########
@@ -154,6 +154,7 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |remainder|remainder(x, y) returns the remainder operation on two arguments as prescribed by the IEEE 754 standard|
 |rint|rint(x) returns value that is closest in value to x and is equal to a mathematical integer|
 |round|round(x, y) returns the value of the x rounded to the y decimal places. While x can be an integer or floating-point number, y must be an integer. The type of the return value is specified by that of x. y defaults to 0 if omitted. When y is negative, x is rounded on the left side of the y decimal points. If x is `NaN`, x returns 0. If x is infinity, x will be converted to the nearest finite double. |
+|safe_divide|safe_divide(x,y) returns the division of x by y if y is not equal to 0. In case y is 0 it returns null or default values (if returning default values are enabled). |

Review comment:
       Done




-- 
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@druid.apache.org

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



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

[GitHub] [druid] suneet-s closed pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
suneet-s closed pull request #11904:
URL: https://github.com/apache/druid/pull/11904


   


-- 
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@druid.apache.org

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



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

[GitHub] [druid] somu-imply commented on a change in pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
somu-imply commented on a change in pull request #11904:
URL: https://github.com/apache/druid/pull/11904#discussion_r748567555



##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -1165,6 +1165,46 @@ protected ExprEval eval(double param)
     }
   }
 
+  class SafeDivide extends BivariateMathFunction
+  {
+    public static final String NAME = "safe_divide";
+
+    @Override
+    public String name()
+    {
+      return NAME ;
+    }
+
+    @Nullable
+    @Override
+    public ExpressionType getOutputType(Expr.InputBindingInspector inspector, List<Expr> args)
+    {
+      return ExpressionTypeConversion.integerMathFunction(
+          args.get(0).getOutputType(inspector),
+          args.get(1).getOutputType(inspector)
+      );
+    }
+
+    @Override
+    protected ExprEval eval(final long x, final long y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }
+      return ExprEval.of(x / y);
+    }
+
+    @Override
+    protected ExprEval eval(final double x, final double y)
+    {
+      if(y==0) {
+        return ExprEval.of(0);
+      }

Review comment:
       Change made. Added a case and testcases where x or y is NaN or +- Infinity

##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -732,6 +732,16 @@ public void testSizeFormatInvalidArgumentSize()
           .eval(bindings);
   }
 
+  @Test
+  public void testSafeDivide()
+  {
+    // happy path maths
+    assertExpr("safe_divide(3, 1)", 3L);
+    assertExpr("safe_divide(4.5, 2)", 2.25);
+    assertExpr("safe_divide(3, 0)", 0L);
+    assertExpr("safe_divide(3.7, 0.0)", 0L);

Review comment:
       Done




-- 
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@druid.apache.org

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



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

[GitHub] [druid] suneet-s merged pull request #11904: Adding safe divide function

Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #11904:
URL: https://github.com/apache/druid/pull/11904


   


-- 
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@druid.apache.org

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



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