Adding safe divide function by somu-imply · Pull Request #11904 · apache/druid

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

 |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

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

 |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

+      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

+      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

+    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

 |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

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

clintropolis

looks like a couple of legit ci failures still

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

 |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

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

+      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

+      .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 documentation updates * Changing based on review comments * Addressing review comments, fixing coding style, docs and spelling * Checkstyle passes for all code * Fixing expected results for infinity * Revert "Fixing expected results for infinity" This reverts commit 5fd5cd4. * Updating test result and a space in docs

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

coderabbitai Bot mentioned this pull request Jun 1, 2026

feat: dbt PR review — layered deterministic + advisory reviewer for dbt/SQL AltimateAI/altimate-code#856

Merged

11 tasks

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

Uh oh!

Conversation

somu-imply commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Key changed/added classes in this PR

Uh oh!

suneet-s commented Nov 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

somu-imply commented Nov 11, 2021 •

edited

Loading

clintropolis left a comment •

edited

Loading