zabetak commented on code in PR #4653:
URL: https://github.com/apache/calcite/pull/4653#discussion_r2581046275
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java:
##########
@@ -664,6 +664,13 @@ enum SelectAliasLookup {
*/
boolean checkedArithmetic();
+ /**
+ * Whether the implementation uses the nullable form of DIVIDE.
+ * Most SQL dialects will produce a runtime exception on division by zero,
+ * but some dialects return NULL instead (e.g. sqlite).
+ */
+ boolean nullableDivide();
Review Comment:
Should we make this a `default` method otherwise this becomes a breaking
change.
##########
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##########
@@ -335,6 +339,16 @@ public enum SqlKind {
*/
CHECKED_DIVIDE,
+ /**
+ * Unchecked nullable version of DIVIDE, which produces NULL when dividing
by zero.
+ */
+ DIVIDE_0_NULL,
+
+ /**
+ * Checked nullable version of DIVIDE, which produces NULL when dividing by
zero.
+ */
+ CHECKED_DIVIDE_0_NULL,
+
Review Comment:
Do we need to introduce a new `SqlKind` entry for each division variant?
It's true that we have used this pattern before but maybe its not necessary in
every case. In some cases the exact division peculiarities may not be that
important so it could be fine to keep the existing `SqlKind.DIVIDE`.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeTransforms.java:
##########
@@ -98,6 +98,17 @@ public abstract class SqlTypeTransforms {
opBinding.getTypeFactory().createTypeWithNullability(
requireNonNull(typeToTransform, "typeToTransform"), true);
+ /**
+ * Parameter type-inference transform strategy where a derived type is
+ * transformed into the same type with nulls allowed if the result is not
floating point.
+ */
+ public static final SqlTypeTransform FORCE_NULLABLE_NON_FP =
+ (opBinding, typeToTransform) -> {
+ boolean nullable =
!SqlTypeName.APPROX_TYPES.contains(typeToTransform.getSqlTypeName());
Review Comment:
For approximate types `nullable` becomes `false` which will end up making
the resulting type non-nullable which I don't think its correct. I have the
impression that the result of the division is always nullable and this is also
unrelated to the fact that we divide by zero or not.
**Example**
```sql
2.5/null
null/2.5
(float) null/ (float) null
```
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -2823,6 +2823,147 @@ public static long divide(long b0, BigDecimal b1) {
:
ULong.valueOf(UnsignedType.toBigInteger(b0).divide(UnsignedType.toBigInteger(b1)));
}
+ // nullable divide
+
+ public static @Nullable Integer divide0Null(int b0, BigDecimal b1) {
+ if (b1.equals(BigDecimal.ZERO)) {
+ return null;
+ }
+ return BigDecimal.valueOf(b0)
+ .divide(b1, RoundingMode.HALF_DOWN).intValue();
+ }
+
+ public static @Nullable Long divide0Null(long b0, BigDecimal b1) {
+ if (b1.equals(BigDecimal.ZERO)) {
+ return null;
+ }
+ return BigDecimal.valueOf(b0)
+ .divide(b1, RoundingMode.HALF_DOWN).longValue();
+ }
+
+ public static @Nullable UByte divide0Null(@PolyNull UByte b0,
+ @PolyNull UByte b1) {
+ if (b0 == null || b1 == null) {
+ return castNonNull(null);
+ } else if (b1.intValue() == 0) {
+ return null;
+ } else {
+ return UByte.valueOf(b0.intValue() / b1.intValue());
+ }
+ }
+
+ public static @Nullable UShort divide0Null(@PolyNull UShort b0,
+ @PolyNull UShort b1) {
+ if (b0 == null || b1 == null) {
+ return null;
+ } else if (b1.intValue() == 0) {
+ return null;
+ } else {
+ return UShort.valueOf(b0.intValue() / b1.intValue());
+ }
+ }
+
+ public static @Nullable UInteger divide0Null(@PolyNull UInteger b0,
+ @PolyNull UInteger b1) {
+ if (b0 == null || b1 == null) {
+ return null;
+ } else if (b1.longValue() == 0L) {
+ return null;
+ } else {
+ return UInteger.valueOf(b0.longValue() / b1.longValue());
+ }
+ }
+
+ public static @Nullable ULong divide0Null(@PolyNull ULong b0,
+ @PolyNull ULong b1) {
+ if (b0 == null || b1 == null) {
+ return null;
+ } else if (b1.equals(ULong.valueOf(0))) {
+ return null;
+ } else {
+ return
ULong.valueOf(UnsignedType.toBigInteger(b0).divide(UnsignedType.toBigInteger(b1)));
+ }
+ }
+
+ /** SQL <code>/</code> operator applied to int values. */
+ public static @Nullable Integer divide0Null(int b0, int b1) {
+ if (b1 == 0) {
+ return null;
+ } else {
+ return b0 / b1;
+ }
+ }
+
+ /** SQL <code>/</code> operator applied to int values; left side may be
+ * null. */
+ public static @Nullable Integer divide0Null(@PolyNull Integer b0, int b1) {
+ if (b0 == null) {
+ return null;
+ } else if (b1 == 0) {
+ return null;
+ } else {
+ return b0 / b1;
+ }
+ }
+
+ /** SQL <code>/</code> operator applied to int values; right side may be
+ * null. */
+ public static @Nullable Integer divide0Null(int b0, @PolyNull Integer b1) {
+ if (b1 == null) {
+ return null;
+ } else if (b1 == 0) {
+ return null;
+ } else {
+ return b0 / b1;
+ }
+ }
+
+ /** SQL <code>/</code> operator applied to nullable int values. */
+ public static @Nullable Integer divide0Null(@PolyNull Integer b0,
+ @PolyNull Integer b1) {
+ if (b0 == null || b1 == null) {
+ return null;
+ } else if (b1 == 0) {
+ return null;
+ } else {
+ return b0 / b1;
+ }
+ }
+
+ /** SQL <code>/</code> operator applied to nullable long and int values. */
+ public static @Nullable Long divide0Null(Long b0, @PolyNull Integer b1) {
Review Comment:
How do we decide when to use the `@PolyNull` annotation? Why we don't use if
for `Long`?
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2467,10 +2477,62 @@ FROM "hr"."emps";
!ok
-# [CALCITE-6566] JDBC adapter should generate PI function with parentheses in
most dialects
-
!use scott-mysql
+# [CALCITE-7270] Add support for a DIVIDE_0_NULL operator
Review Comment:
Since we are testing specific operator(s) the `operator.iq` file seems to be
a better match for such tests.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -839,8 +845,13 @@ void populate1() {
defineBinary(CHECKED_DIVIDE, DivideChecked, NullPolicy.STRICT,
"checkedDivide");
defineBinary(CHECKED_DIVIDE_INTEGER, DivideChecked, NullPolicy.STRICT,
"checkedDivide");
defineUnary(CHECKED_UNARY_MINUS, NegateChecked, NullPolicy.STRICT,
"checkedUnaryMinus");
+ // nullable division
+ defineBinary(DIVIDE_0_NULL, Divide0Null, NullPolicy.SEMI_STRICT,
"nullableDivide");
Review Comment:
`DIVIDE_0_NULL` is already defined in line 836. Why do we need two
implementors for the same operator?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java:
##########
@@ -1872,10 +1901,26 @@ public class SqlStdOperatorTable extends
ReflectiveSqlOperatorTable {
// Return type is same as divisor (2nd operand)
// SQL2003 Part2 Section 6.27, Syntax Rules 9
SqlBasicFunction.create(SqlKind.MOD,
+ // A rather unfortunate name for this return type strategy
ReturnTypes.NULLABLE_MOD,
OperandTypes.EXACT_NUMERIC_EXACT_NUMERIC)
.withFunctionType(SqlFunctionCategory.NUMERIC);
+ /**
+ * Variant of arithmetic remainder function {@code MOD} which returns NULL
when
+ * the denominator is 0.
+ */
+ public static final SqlFunction MOD_0_NULL =
+ // Return type is same as divisor (2nd operand)
+ // SQL2003 Part2 Section 6.27, Syntax Rules 9
+ // Unfortunately there cannot exist two functions in the standard
operator
+ // table with the same exact name, so we need to use a different name,
+ // although in SQL this would be shown just as MOD
+ SqlBasicFunction.create("NULLABLE_MOD", SqlKind.MOD_0_NULL,
Review Comment:
There is also `PERCENT_REMAINDER` with name `%` why did we pick
"NULLABLE_MOD" as a name?
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -355,6 +355,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_MINUS_PREFIX, Policy.ANY);
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
+ map.put(SqlKind.DIVIDE_0_NULL, Policy.AS_IS);
+ map.put(SqlKind.MOD_0_NULL, Policy.AS_IS);
Review Comment:
Should we also do something about `CHECKED_DIVIDE_0_NULL`?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -327,6 +327,8 @@ RexNode simplify(RexNode e, RexUnknownAs unknownAs) {
case MINUS:
case TIMES:
case DIVIDE:
+ case DIVIDE_0_NULL:
+ case CHECKED_DIVIDE_0_NULL:
Review Comment:
The fact that we don't have separate handling for `DIVIDE_0_NULL` and
`CHECKED_DIVIDE_0_NULL` anywhere in the code is a hint that maybe we don't need
separate entries in `SqlKind`.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionType.java:
##########
@@ -146,12 +146,30 @@ public enum ExpressionType {
*/
DivideChecked(" / ", false, 3, false),
+ /**
+ * A nullable division operation, such as (a / b), for numeric
+ * operands, but which returns NULL for a 0 denominator.
+ */
+ Divide0Null(" / ", false, 3, false),
+
+ /**
+ * A checked nullable division operation, such as (a / b), for numeric
+ * operands which returns NULL for a 0 denominator.
+ */
+ Divide0NullChecked(" / ", false, 3, false),
+
/**
* A percent remainder operation, such as (a % b), for numeric
* operands.
*/
Mod(" % ", false, 3, false),
+ /**
+ * A percent remainder operation, such as (a % b), for numeric
+ * operands.
Review Comment:
Javadoc is identical with above
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3906,6 +4047,54 @@ public static BigDecimal mod(BigDecimal b0, BigDecimal
b1) {
return bigDecimals[1];
}
+ /** SQL nullable <code>MOD</code> operator applied to byte values. */
+ public static @Nullable Byte mod0Null(byte b0, byte b1) {
+ if (b1 == 0) {
+ return null;
+ }
+ return (byte) (b0 % b1);
Review Comment:
nit: slightly shorter
```java
return b1 == 0 ? null : (byte) (b0 % b1)
```
##########
site/_docs/reference.md:
##########
@@ -1428,6 +1428,7 @@ comp:
| POWER(numeric1, numeric2) | Returns *numeric1* raised to the power of
*numeric2*
| ABS(numeric) | Returns the absolute value of *numeric*
| MOD(numeric1, numeric2) | Returns the remainder (modulus) of *numeric1*
divided by *numeric2*. The result is negative only if *numeric1* is negative
+| NULLABLE_MOD(numeric1, numeric2) | Returns the remainder (modulus) of
*numeric1* divided by *numeric2*. The result is negative only if *numeric1* is
negative. Returns NULL when numeric2 is zero
Review Comment:
Should we document somewhere above the available division variants?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3906,6 +4047,54 @@ public static BigDecimal mod(BigDecimal b0, BigDecimal
b1) {
return bigDecimals[1];
}
+ /** SQL nullable <code>MOD</code> operator applied to byte values. */
+ public static @Nullable Byte mod0Null(byte b0, byte b1) {
+ if (b1 == 0) {
+ return null;
+ }
+ return (byte) (b0 % b1);
+ }
+
+ /** SQL nullable <code>MOD</code> operator applied to short values. */
+ public static @Nullable Short mod0Null(short b0, short b1) {
+ if (b1 == 0) {
+ return null;
+ }
+ return (short) (b0 % b1);
+ }
+
+ /** SQL nullable <code>MOD</code> operator applied to int values. */
+ public static @Nullable Integer mod0Null(int b0, int b1) {
+ if (b1 == 0) {
+ return null;
+ }
+ return b0 % b1;
+ }
+
+ /** SQL nullable <code>MOD</code> operator applied to long values. */
+ public static @Nullable Long mod0Null(long b0, long b1) {
+ if (b1 == 0L) {
+ return null;
+ }
+ return b0 % b1;
+ }
+
+ public static @Nullable BigDecimal mod0Null(BigDecimal b0, int b1) {
+ return mod0Null(b0, BigDecimal.valueOf(b1));
+ }
+
+ public static @Nullable BigDecimal mod0Null(int b0, BigDecimal b1) {
+ return mod(BigDecimal.valueOf(b0), b1);
Review Comment:
The check for b1 == 0 seems to be missing from here since we delegate to
`mod` and **not** `mod0Null`. Is there adequate test coverage?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3906,6 +4047,54 @@ public static BigDecimal mod(BigDecimal b0, BigDecimal
b1) {
return bigDecimals[1];
}
+ /** SQL nullable <code>MOD</code> operator applied to byte values. */
+ public static @Nullable Byte mod0Null(byte b0, byte b1) {
+ if (b1 == 0) {
+ return null;
+ }
+ return (byte) (b0 % b1);
+ }
+
+ /** SQL nullable <code>MOD</code> operator applied to short values. */
+ public static @Nullable Short mod0Null(short b0, short b1) {
Review Comment:
In contrast with `divide0Null` it seems that we have less variants based on
combinations of data types. Is this normal/expected?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -2823,6 +2823,147 @@ public static long divide(long b0, BigDecimal b1) {
:
ULong.valueOf(UnsignedType.toBigInteger(b0).divide(UnsignedType.toBigInteger(b1)));
}
+ // nullable divide
+
+ public static @Nullable Integer divide0Null(int b0, BigDecimal b1) {
+ if (b1.equals(BigDecimal.ZERO)) {
+ return null;
+ }
+ return BigDecimal.valueOf(b0)
+ .divide(b1, RoundingMode.HALF_DOWN).intValue();
+ }
+
+ public static @Nullable Long divide0Null(long b0, BigDecimal b1) {
+ if (b1.equals(BigDecimal.ZERO)) {
+ return null;
+ }
+ return BigDecimal.valueOf(b0)
+ .divide(b1, RoundingMode.HALF_DOWN).longValue();
+ }
+
+ public static @Nullable UByte divide0Null(@PolyNull UByte b0,
+ @PolyNull UByte b1) {
+ if (b0 == null || b1 == null) {
+ return castNonNull(null);
Review Comment:
Is the `castNonNull` necessary? Why? I checked other similar code but can't
tell what's the usage rule.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -624,6 +625,7 @@ public RelRoot convertQuery(
final boolean needsValidation,
final boolean top) {
final boolean unwrapMeasures = !validator().config().embeddedQuery();
+ query = implementArithmetic(validator().config().conformance(), query);
Review Comment:
There is another pattern for introducing operators based on conformance that
doesn't require multiple passes and modifications of the AST. See
`SqlStdOperatorTable` and in particular the changes around CALCITE-5747,
CALCITE-6730, etc.
##########
core/src/main/java/org/apache/calcite/sql2rel/ConvertDivideToNullable.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.calcite.sql2rel;
+
+import org.apache.calcite.sql.SqlBasicCall;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/**
+ * Converts a RelNode tree such that division uses nullable division, which
+ * produces NULL on division by zero. Since there is no type information
available,
+ * we convert all instances of DIVIDE and MOD operators. However, operators
+ * acting on floating point values or intervals should be converted back.
+ */
+public class ConvertDivideToNullable extends SqlShuttle {
Review Comment:
We don't necessarily need a shuttle. More details in the comments in
`SqlToRelConverter`.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionType.java:
##########
@@ -146,12 +146,30 @@ public enum ExpressionType {
*/
DivideChecked(" / ", false, 3, false),
+ /**
Review Comment:
Why do we need all these new entries here. I didn't see any conditionals
based on those?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java:
##########
@@ -1872,10 +1901,26 @@ public class SqlStdOperatorTable extends
ReflectiveSqlOperatorTable {
// Return type is same as divisor (2nd operand)
// SQL2003 Part2 Section 6.27, Syntax Rules 9
SqlBasicFunction.create(SqlKind.MOD,
+ // A rather unfortunate name for this return type strategy
Review Comment:
What's the problem with the name?
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2467,10 +2477,62 @@ FROM "hr"."emps";
!ok
-# [CALCITE-6566] JDBC adapter should generate PI function with parentheses in
most dialects
-
!use scott-mysql
+# [CALCITE-7270] Add support for a DIVIDE_0_NULL operator
+select 5 / 0 as a;
++---+
+| A |
++---+
+| |
++---+
+(1 row)
+
+!ok
+
+# No NULL result produced for double division
+select CAST(5 AS DOUBLE) / 0 as a;
++----------+
+| A |
++----------+
+| Infinity |
++----------+
+(1 row)
+
+!ok
+
+select 5 / 1 as a;
++---+
+| A |
++---+
+| 5 |
++---+
+(1 row)
+
+!ok
+
+# [CALCITE-7270] Add support for a DIVIDE_0_NULL operator
Review Comment:
nit: In some queries there is a comment with the JIRA ticket and summary and
in others there isn't.
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -355,6 +355,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_MINUS_PREFIX, Policy.ANY);
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
+ map.put(SqlKind.DIVIDE_0_NULL, Policy.AS_IS);
+ map.put(SqlKind.MOD_0_NULL, Policy.AS_IS);
Review Comment:
We could override the `SqlOperator.getStrongPolicyInference` for the new
operators and avoid the need for introducing a new SqlKind entry.
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -18,6 +18,16 @@
!use post
!set outputformat mysql
+select CAST(5 AS DOUBLE) / 0 as a;
Review Comment:
For the record, Apache Hive also returns `NULL` whenever you divide by zero
no matter the data type.
```sql
CREATE TABLE types
(
cint INT,
cbint BIGINT,
cfloat FLOAT,
cdouble DOUBLE,
cdecimal DECIMAL(10, 2)
);
INSERT INTO types
VALUES (5, 5, 5.5, 5.5, 5.5);
SELECT cint / 0 AS cint_div_zero,
cbint / 0 AS cbint_div_zero,
cfloat / 0 AS cfloat_div_zero,
cdouble / 0 AS cdouble_div_zero,
cdecimal / 0 AS cdecimal_div_zero
```
```
NULL NULL NULL NULL NULL
```
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java:
##########
@@ -1872,10 +1901,26 @@ public class SqlStdOperatorTable extends
ReflectiveSqlOperatorTable {
// Return type is same as divisor (2nd operand)
// SQL2003 Part2 Section 6.27, Syntax Rules 9
SqlBasicFunction.create(SqlKind.MOD,
+ // A rather unfortunate name for this return type strategy
ReturnTypes.NULLABLE_MOD,
OperandTypes.EXACT_NUMERIC_EXACT_NUMERIC)
.withFunctionType(SqlFunctionCategory.NUMERIC);
+ /**
+ * Variant of arithmetic remainder function {@code MOD} which returns NULL
when
+ * the denominator is 0.
+ */
+ public static final SqlFunction MOD_0_NULL =
+ // Return type is same as divisor (2nd operand)
+ // SQL2003 Part2 Section 6.27, Syntax Rules 9
+ // Unfortunately there cannot exist two functions in the standard
operator
+ // table with the same exact name, so we need to use a different name,
Review Comment:
Is this statement really true? How come we don't have the same problem for
division?
--
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]
