Skip to content

Commit 8714c88

Browse files
penghuopenghuo
committed
Address review feedback: NonFallbackCalciteException, type coercion, and integration tests
- Wrap ArithmeticException in NonFallbackCalciteException to prevent silent V2 fallback on overflow - Use MathUtils.coerceToWidestIntegralType for consistent type handling - Add missing unit test cases: subtraction positive overflow, multiply negative overflow, mixed int/long for subtract and multiply - Add CalciteArithmeticOverflowIT end-to-end integration test with boundary-value data (Integer.MAX_VALUE, Long.MAX_VALUE) exercising overflow through the full PPL pipeline Signed-off-by: developer <developer@example.com> Signed-off-by: Peng Huo <penghuo@gmail.com>
1 parent 3229033 commit 8714c88

File tree

5 files changed

+246
-32
lines changed

5 files changed

+246
-32
lines changed

‎core/src/main/java/org/opensearch/sql/expression/function/udf/math/AddFunction.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.apache.calcite.sql.type.SqlTypeTransforms;
1919
import org.opensearch.sql.calcite.utils.MathUtils;
2020
import org.opensearch.sql.calcite.utils.PPLOperandTypes;
21+
import org.opensearch.sql.exception.NonFallbackCalciteException;
2122
import org.opensearch.sql.expression.function.ImplementorUDF;
2223
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2324

@@ -52,12 +53,20 @@ public Expression implement(
5253
}
5354

5455
public static Number add(Number left, Number right) {
55-
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
56-
if (left instanceof Long || right instanceof Long) {
57-
return Math.addExact(left.longValue(), right.longValue());
56+
try {
57+
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
58+
if (left instanceof Long || right instanceof Long) {
59+
return MathUtils.coerceToWidestIntegralType(
60+
left, right, Math.addExact(left.longValue(), right.longValue()));
61+
}
62+
return MathUtils.coerceToWidestIntegralType(
63+
left, right, Math.addExact(left.intValue(), right.intValue()));
5864
}
59-
return Math.addExact(left.intValue(), right.intValue());
60-
} else if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
65+
} catch (ArithmeticException e) {
66+
throw new NonFallbackCalciteException(
67+
"integer overflow in addition: " + left + " + " + right, e);
68+
}
69+
if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
6170
BigDecimal b0 =
6271
left instanceof BigDecimal
6372
? (BigDecimal) left

‎core/src/main/java/org/opensearch/sql/expression/function/udf/math/MultiplyFunction.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.apache.calcite.sql.type.SqlTypeTransforms;
1919
import org.opensearch.sql.calcite.utils.MathUtils;
2020
import org.opensearch.sql.calcite.utils.PPLOperandTypes;
21+
import org.opensearch.sql.exception.NonFallbackCalciteException;
2122
import org.opensearch.sql.expression.function.ImplementorUDF;
2223
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2324

@@ -52,12 +53,20 @@ public Expression implement(
5253
}
5354

5455
public static Number multiply(Number left, Number right) {
55-
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
56-
if (left instanceof Long || right instanceof Long) {
57-
return Math.multiplyExact(left.longValue(), right.longValue());
56+
try {
57+
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
58+
if (left instanceof Long || right instanceof Long) {
59+
return MathUtils.coerceToWidestIntegralType(
60+
left, right, Math.multiplyExact(left.longValue(), right.longValue()));
61+
}
62+
return MathUtils.coerceToWidestIntegralType(
63+
left, right, Math.multiplyExact(left.intValue(), right.intValue()));
5864
}
59-
return Math.multiplyExact(left.intValue(), right.intValue());
60-
} else if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
65+
} catch (ArithmeticException e) {
66+
throw new NonFallbackCalciteException(
67+
"integer overflow in multiplication: " + left + " * " + right, e);
68+
}
69+
if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
6170
BigDecimal b0 =
6271
left instanceof BigDecimal
6372
? (BigDecimal) left

‎core/src/main/java/org/opensearch/sql/expression/function/udf/math/SubtractFunction.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.apache.calcite.sql.type.SqlTypeTransforms;
1919
import org.opensearch.sql.calcite.utils.MathUtils;
2020
import org.opensearch.sql.calcite.utils.PPLOperandTypes;
21+
import org.opensearch.sql.exception.NonFallbackCalciteException;
2122
import org.opensearch.sql.expression.function.ImplementorUDF;
2223
import org.opensearch.sql.expression.function.UDFOperandMetadata;
2324

@@ -52,12 +53,20 @@ public Expression implement(
5253
}
5354

5455
public static Number subtract(Number left, Number right) {
55-
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
56-
if (left instanceof Long || right instanceof Long) {
57-
return Math.subtractExact(left.longValue(), right.longValue());
56+
try {
57+
if (MathUtils.isIntegral(left) && MathUtils.isIntegral(right)) {
58+
if (left instanceof Long || right instanceof Long) {
59+
return MathUtils.coerceToWidestIntegralType(
60+
left, right, Math.subtractExact(left.longValue(), right.longValue()));
61+
}
62+
return MathUtils.coerceToWidestIntegralType(
63+
left, right, Math.subtractExact(left.intValue(), right.intValue()));
5864
}
59-
return Math.subtractExact(left.intValue(), right.intValue());
60-
} else if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
65+
} catch (ArithmeticException e) {
66+
throw new NonFallbackCalciteException(
67+
"integer overflow in subtraction: " + left + " - " + right, e);
68+
}
69+
if (MathUtils.isDecimal(left) || MathUtils.isDecimal(right)) {
6170
BigDecimal b0 =
6271
left instanceof BigDecimal
6372
? (BigDecimal) left

‎core/src/test/java/org/opensearch/sql/expression/function/udf/math/OverflowSafeArithmeticTests.java‎

Lines changed: 57 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.junit.jupiter.api.DisplayNameGenerator;
1414
import org.junit.jupiter.api.Nested;
1515
import org.junit.jupiter.api.Test;
16+
import org.opensearch.sql.exception.NonFallbackCalciteException;
1617

1718
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
1819
class OverflowSafeArithmeticTests {
@@ -30,30 +31,30 @@ void long_addition_returns_correct_result() {
3031
}
3132

3233
@Test
33-
void integer_overflow_throws_arithmetic_exception() {
34+
void integer_overflow_throws() {
3435
assertThrows(
35-
ArithmeticException.class,
36+
NonFallbackCalciteException.class,
3637
() -> AddFunction.AddImplementor.add(Integer.MAX_VALUE, 1));
3738
}
3839

3940
@Test
40-
void integer_negative_overflow_throws_arithmetic_exception() {
41+
void integer_negative_overflow_throws() {
4142
assertThrows(
42-
ArithmeticException.class,
43+
NonFallbackCalciteException.class,
4344
() -> AddFunction.AddImplementor.add(Integer.MIN_VALUE, -1));
4445
}
4546

4647
@Test
47-
void long_overflow_throws_arithmetic_exception() {
48+
void long_overflow_throws() {
4849
assertThrows(
49-
ArithmeticException.class,
50+
NonFallbackCalciteException.class,
5051
() -> AddFunction.AddImplementor.add(Long.MAX_VALUE, 1L));
5152
}
5253

5354
@Test
54-
void long_negative_overflow_throws_arithmetic_exception() {
55+
void long_negative_overflow_throws() {
5556
assertThrows(
56-
ArithmeticException.class,
57+
NonFallbackCalciteException.class,
5758
() -> AddFunction.AddImplementor.add(Long.MIN_VALUE, -1L));
5859
}
5960

@@ -75,7 +76,6 @@ void decimal_addition_returns_big_decimal() {
7576

7677
@Test
7778
void mixed_integer_long_uses_long_arithmetic() {
78-
// int + long should use long arithmetic
7979
Number result = AddFunction.AddImplementor.add(Integer.MAX_VALUE, 1L);
8080
assertEquals((long) Integer.MAX_VALUE + 1L, result);
8181
}
@@ -94,19 +94,33 @@ void long_subtraction_returns_correct_result() {
9494
}
9595

9696
@Test
97-
void integer_overflow_throws_arithmetic_exception() {
97+
void integer_negative_overflow_throws() {
9898
assertThrows(
99-
ArithmeticException.class,
99+
NonFallbackCalciteException.class,
100100
() -> SubtractFunction.SubtractImplementor.subtract(Integer.MIN_VALUE, 1));
101101
}
102102

103103
@Test
104-
void long_overflow_throws_arithmetic_exception() {
104+
void integer_positive_overflow_throws() {
105105
assertThrows(
106-
ArithmeticException.class,
106+
NonFallbackCalciteException.class,
107+
() -> SubtractFunction.SubtractImplementor.subtract(Integer.MAX_VALUE, -1));
108+
}
109+
110+
@Test
111+
void long_negative_overflow_throws() {
112+
assertThrows(
113+
NonFallbackCalciteException.class,
107114
() -> SubtractFunction.SubtractImplementor.subtract(Long.MIN_VALUE, 1L));
108115
}
109116

117+
@Test
118+
void long_positive_overflow_throws() {
119+
assertThrows(
120+
NonFallbackCalciteException.class,
121+
() -> SubtractFunction.SubtractImplementor.subtract(Long.MAX_VALUE, -1L));
122+
}
123+
110124
@Test
111125
void double_subtraction_does_not_throw() {
112126
assertEquals(1.0, SubtractFunction.SubtractImplementor.subtract(3.0, 2.0));
@@ -118,6 +132,12 @@ void decimal_subtraction_returns_big_decimal() {
118132
SubtractFunction.SubtractImplementor.subtract(BigDecimal.TEN, BigDecimal.ONE);
119133
assertEquals(new BigDecimal("9"), result);
120134
}
135+
136+
@Test
137+
void mixed_integer_long_uses_long_arithmetic() {
138+
Number result = SubtractFunction.SubtractImplementor.subtract(1, 2L);
139+
assertEquals(1L - 2L, result);
140+
}
121141
}
122142

123143
@Nested
@@ -133,19 +153,33 @@ void long_multiplication_returns_correct_result() {
133153
}
134154

135155
@Test
136-
void integer_overflow_throws_arithmetic_exception() {
156+
void integer_overflow_throws() {
137157
assertThrows(
138-
ArithmeticException.class,
158+
NonFallbackCalciteException.class,
139159
() -> MultiplyFunction.MultiplyImplementor.multiply(Integer.MAX_VALUE, 2));
140160
}
141161

142162
@Test
143-
void long_overflow_throws_arithmetic_exception() {
163+
void integer_negative_overflow_throws() {
144164
assertThrows(
145-
ArithmeticException.class,
165+
NonFallbackCalciteException.class,
166+
() -> MultiplyFunction.MultiplyImplementor.multiply(Integer.MIN_VALUE, 2));
167+
}
168+
169+
@Test
170+
void long_overflow_throws() {
171+
assertThrows(
172+
NonFallbackCalciteException.class,
146173
() -> MultiplyFunction.MultiplyImplementor.multiply(Long.MAX_VALUE, 2L));
147174
}
148175

176+
@Test
177+
void long_negative_overflow_throws() {
178+
assertThrows(
179+
NonFallbackCalciteException.class,
180+
() -> MultiplyFunction.MultiplyImplementor.multiply(Long.MIN_VALUE, 2L));
181+
}
182+
149183
@Test
150184
void double_multiplication_does_not_throw() {
151185
assertEquals(6.0, MultiplyFunction.MultiplyImplementor.multiply(2.0, 3.0));
@@ -157,5 +191,11 @@ void decimal_multiplication_returns_big_decimal() {
157191
MultiplyFunction.MultiplyImplementor.multiply(new BigDecimal("2"), new BigDecimal("3"));
158192
assertEquals(new BigDecimal("6"), result);
159193
}
194+
195+
@Test
196+
void mixed_integer_long_uses_long_arithmetic() {
197+
Number result = MultiplyFunction.MultiplyImplementor.multiply(2, 3L);
198+
assertEquals(6L, result);
199+
}
160200
}
161201
}

0 commit comments

Comments
 (0)