Adding safe divide function #11904

somu-imply · 2021-11-11T00:36:08Z

Adds a Safe_Divide function

Description

Added a new safe divide function that is guarded against division by 0. Safe_divide(x,y) returns x/y if y is not equal to 0. It returns 0 otherwise.

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Key changed/added classes in this PR

Safe_Divide function
Calcite layer direct operator to connect with newly created safe_divide native function

This PR has:

been self-reviewed.
- using the concurrency checklist (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
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 is met.
added integration tests.
been tested in a test Druid cluster.

…mentation updates

suneet-s · 2021-11-11T01:58:18Z

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

FrankChen021 · 2021-11-11T02:15:03Z

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|


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

What if either x or y is NULL?

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)

FrankChen021 · 2021-11-11T02:16:10Z

docs/querying/sql.md

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


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

Suggested change

|`SAFE_DIVIDE(x,y)`|Returns the division of x by y guarded on division by 0 |

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

FrankChen021 · 2021-11-11T02:17:03Z

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|


What if either x or y is NULL?

clintropolis

nice, overall lgtm, just a few minor comments (and noticed a few minor formatting issues that checkstyle will probably flag) 👍

clintropolis · 2021-11-11T04:37:14Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+      if(y==0) {
+        return ExprEval.of(0);
+      }


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

Suggested change

if(y==0) {

return ExprEval.of(0);

}

if (y == 0 && x != 0) {

return ExprEval.ofLong(NullHandling.defaultLongValue());

}

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

clintropolis · 2021-11-11T04:38:08Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+      if(y==0) {
+        return ExprEval.of(0);
+      }


same suggestion (and also should preserve double typing)

Suggested change

if(y==0) {

return ExprEval.of(0);

}

if (y == 0 && x != 0) {

return ExprEval.ofDouble(NullHandling.defaultDoubleValue());

}

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

clintropolis · 2021-11-11T04:41:52Z

core/src/test/java/org/apache/druid/math/expr/FunctionTest.java

+    assertExpr("safe_divide(3, 0)", 0L);
+    assertExpr("safe_divide(3.7, 0.0)", 0L);


if other suggestion is applied:

Suggested change

assertExpr("safe_divide(3, 0)", 0L);

assertExpr("safe_divide(3.7, 0.0)", 0L);

assertExpr("safe_divide(3, 0)", NullHandling.defaultLongValue());

assertExpr("safe_divide(3.7, 0.0)", NullHandling.defaultDoubleValue());

clintropolis · 2021-11-11T04:42:58Z

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|


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)

clintropolis · 2021-11-11T04:43:29Z

.../main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java

@@ -0,0 +1,34 @@
+package org.apache.druid.sql.calcite.expression.builtin;


missing apache license header, can copy from any other file

License headed added, alphabetical order maintained, null handling info added

core/src/main/java/org/apache/druid/math/expr/Function.java

clintropolis

looks like a couple of legit ci failures still

clintropolis · 2021-11-13T01:00:58Z

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


nit: to match other docs that describe difference between default and sql compatible null handling mode

Suggested change

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

|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

clintropolis · 2021-11-13T01:23:45Z

docs/querying/sql.md

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


Suggested change

|`SAFE_DIVIDE(x,y)`|Returns the division of x by y guarded on division by 0 |

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

clintropolis · 2021-11-13T01:26:39Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+      ExpressionType type = ExpressionType.DOUBLE;
+      for (Expr arg : args) {
+        type = ExpressionTypeConversion.function(type, arg.getOutputType(inspector));
+      }
+      return ExpressionType.asArrayType(type);


this shouldn't be array typed

Suggested change

ExpressionType type = ExpressionType.DOUBLE;

for (Expr arg : args) {

type = ExpressionTypeConversion.function(type, arg.getOutputType(inspector));

}

return ExpressionType.asArrayType(type);

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?

clintropolis · 2021-11-13T01:30:54Z

.../main/java/org/apache/druid/sql/calcite/expression/builtin/SafeDivideOperatorConversion.java

+      .operatorBuilder(StringUtils.toUpperCase(Function.SafeDivide.NAME))
+      .operandTypeChecker(OperandTypes.ANY_NUMERIC)
+      .returnTypeInference(ReturnTypes.QUOTIENT_NULLABLE)
+      .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION)


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

This reverts commit 5fd5cd4.

clintropolis

👍 (though try not to auto format entire files when making changes if it makes a lot of noise as it is much harder to review)

IMPLY-4344: Adding safe divide function along with testcases and docu…

b14dcff

…mentation updates

suneet-s changed the title ~~IMPLY-4344: Adding safe divide function along with testcases and docu…~~ Adding safe divide function Nov 11, 2021

suneet-s closed this Nov 11, 2021

suneet-s reopened this Nov 11, 2021

suneet-s added Area - Querying Release Notes labels Nov 11, 2021

FrankChen021 reviewed Nov 11, 2021

View reviewed changes

clintropolis reviewed Nov 11, 2021

View reviewed changes

Changing based on review comments

b2031be

clintropolis reviewed Nov 13, 2021

View reviewed changes

somu-imply added 5 commits November 15, 2021 10:02

Addressing review comments, fixing coding style, docs and spelling

fd7e391

Checkstyle passes for all code

4d64aae

Fixing expected results for infinity

5fd5cd4

Revert "Fixing expected results for infinity"

bf261b1

This reverts commit 5fd5cd4.

Updating test result and a space in docs

631d424

clintropolis approved these changes Nov 16, 2021

View reviewed changes

suneet-s merged commit 2971078 into apache:master Nov 17, 2021

FrankChen021 mentioned this pull request Jan 13, 2022

if(sum(b) = 0, 0, sum(a) / sum(b)) always return 0 #12151

Closed

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request May 25, 2022

[Draft] 0.23.0 Release notes #12510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding safe divide function #11904

Adding safe divide function #11904

somu-imply commented Nov 11, 2021 •

edited

Loading

suneet-s commented Nov 11, 2021

FrankChen021 Nov 11, 2021

FrankChen021 Nov 11, 2021

clintropolis Nov 11, 2021

FrankChen021 Nov 11, 2021

clintropolis Nov 13, 2021

FrankChen021 Nov 11, 2021

clintropolis left a comment

clintropolis Nov 11, 2021

somu-imply Nov 12, 2021

clintropolis Nov 11, 2021

somu-imply Nov 12, 2021

clintropolis Nov 11, 2021

somu-imply Nov 12, 2021

clintropolis Nov 11, 2021

clintropolis Nov 11, 2021

somu-imply Nov 12, 2021

clintropolis left a comment

clintropolis Nov 13, 2021

somu-imply Nov 16, 2021

clintropolis Nov 13, 2021

clintropolis Nov 13, 2021

somu-imply Nov 16, 2021

clintropolis Nov 13, 2021

somu-imply Nov 16, 2021

clintropolis left a comment •

edited

Loading

	\|`SAFE_DIVIDE(x,y)`\|Returns the division of x by y guarded on division by 0 \|
	\|`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` \|

		assertExpr("safe_divide(3, 0)", 0L);
		assertExpr("safe_divide(3.7, 0.0)", 0L);

		@@ -0,0 +1,34 @@
		package org.apache.druid.sql.calcite.expression.builtin;

	\|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). \|
	\|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`. \|

Adding safe divide function #11904

Adding safe divide function #11904

Conversation

somu-imply commented Nov 11, 2021 • edited Loading

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Key changed/added classes in this PR

suneet-s commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis left a comment • edited Loading

Choose a reason for hiding this comment

somu-imply commented Nov 11, 2021 •

edited

Loading

clintropolis left a comment •

edited

Loading