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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
