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

[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