From de131b13abf630ef4733c2a35c9c2d757b6dc992 Mon Sep 17 00:00:00 2001 From: dubin555 Date: Tue, 3 Mar 2026 11:49:19 +0000 Subject: [PATCH 1/2] [core] Fix integer overflow in FieldSumAgg causing silent data corruption FieldSumAgg.agg() silently wraps on overflow for TINYINT, SMALLINT, INTEGER, and BIGINT types. For example, SUM(Byte.MAX_VALUE, 1) returns -128 instead of detecting the overflow. This causes silent data corruption in merge-tree aggregation when accumulator values exceed type bounds. Use Math.addExact/subtractExact/negateExact for INTEGER and BIGINT, and range-checked helpers for TINYINT and SMALLINT, to throw ArithmeticException on overflow instead of silently producing wrong results. Also fix the same issue in retract() and negative() methods. --- .../compact/aggregate/FieldSumAgg.java | 56 +++++++++++++++---- .../aggregate/FieldAggregatorTest.java | 52 +++++++++++++++++ 2 files changed, 96 insertions(+), 12 deletions(-) diff --git a/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java b/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java index 4b3ad12aea17..2408b7203566 100644 --- a/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java +++ b/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java @@ -55,16 +55,16 @@ public Object agg(Object accumulator, Object inputField) { mergeFieldDD.scale()); break; case TINYINT: - sum = (byte) ((byte) accumulator + (byte) inputField); + sum = addExactByte((byte) accumulator, (byte) inputField); break; case SMALLINT: - sum = (short) ((short) accumulator + (short) inputField); + sum = addExactShort((short) accumulator, (short) inputField); break; case INTEGER: - sum = (int) accumulator + (int) inputField; + sum = Math.addExact((int) accumulator, (int) inputField); break; case BIGINT: - sum = (long) accumulator + (long) inputField; + sum = Math.addExact((long) accumulator, (long) inputField); break; case FLOAT: sum = (float) accumulator + (float) inputField; @@ -105,16 +105,16 @@ public Object retract(Object accumulator, Object inputField) { mergeFieldDD.scale()); break; case TINYINT: - sum = (byte) ((byte) accumulator - (byte) inputField); + sum = subtractExactByte((byte) accumulator, (byte) inputField); break; case SMALLINT: - sum = (short) ((short) accumulator - (short) inputField); + sum = subtractExactShort((short) accumulator, (short) inputField); break; case INTEGER: - sum = (int) accumulator - (int) inputField; + sum = Math.subtractExact((int) accumulator, (int) inputField); break; case BIGINT: - sum = (long) accumulator - (long) inputField; + sum = Math.subtractExact((long) accumulator, (long) inputField); break; case FLOAT: sum = (float) accumulator - (float) inputField; @@ -142,13 +142,13 @@ private Object negative(Object value) { return Decimal.fromBigDecimal( decimal.toBigDecimal().negate(), decimal.precision(), decimal.scale()); case TINYINT: - return (byte) -((byte) value); + return subtractExactByte((byte) 0, (byte) value); case SMALLINT: - return (short) -((short) value); + return subtractExactShort((short) 0, (short) value); case INTEGER: - return -((int) value); + return Math.negateExact((int) value); case BIGINT: - return -((long) value); + return Math.negateExact((long) value); case FLOAT: return -((float) value); case DOUBLE: @@ -161,4 +161,36 @@ private Object negative(Object value) { throw new IllegalArgumentException(msg); } } + + private static byte addExactByte(byte a, byte b) { + int result = a + b; + if (result > Byte.MAX_VALUE || result < Byte.MIN_VALUE) { + throw new ArithmeticException("byte overflow"); + } + return (byte) result; + } + + private static short addExactShort(short a, short b) { + int result = a + b; + if (result > Short.MAX_VALUE || result < Short.MIN_VALUE) { + throw new ArithmeticException("short overflow"); + } + return (short) result; + } + + private static byte subtractExactByte(byte a, byte b) { + int result = a - b; + if (result > Byte.MAX_VALUE || result < Byte.MIN_VALUE) { + throw new ArithmeticException("byte overflow"); + } + return (byte) result; + } + + private static short subtractExactShort(short a, short b) { + int result = a - b; + if (result > Short.MAX_VALUE || result < Short.MIN_VALUE) { + throw new ArithmeticException("short overflow"); + } + return (short) result; + } } diff --git a/paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorTest.java b/paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorTest.java index 8541954be827..571bd810a78c 100644 --- a/paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorTest.java @@ -82,6 +82,7 @@ import static org.apache.paimon.utils.ThetaSketch.sketchOf; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -481,6 +482,57 @@ public void testFieldSumDoubleAgg() { assertThat(fieldSumAgg.retract(null, (double) 5)).isEqualTo((double) -5); } + @Test + public void testFieldSumByteOverflow() { + FieldSumAgg fieldSumAgg = new FieldSumAggFactory().create(new TinyIntType(), null, null); + // Normal case still works + assertThat(fieldSumAgg.agg((byte) 1, (byte) 2)).isEqualTo((byte) 3); + // Overflow should throw ArithmeticException instead of silently wrapping + assertThatThrownBy(() -> fieldSumAgg.agg(Byte.MAX_VALUE, (byte) 1)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.agg(Byte.MIN_VALUE, (byte) -1)) + .isInstanceOf(ArithmeticException.class); + // Retract overflow + assertThatThrownBy(() -> fieldSumAgg.retract(Byte.MIN_VALUE, (byte) 1)) + .isInstanceOf(ArithmeticException.class); + } + + @Test + public void testFieldSumShortOverflow() { + FieldSumAgg fieldSumAgg = new FieldSumAggFactory().create(new SmallIntType(), null, null); + assertThat(fieldSumAgg.agg((short) 1, (short) 2)).isEqualTo((short) 3); + assertThatThrownBy(() -> fieldSumAgg.agg(Short.MAX_VALUE, (short) 1)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.agg(Short.MIN_VALUE, (short) -1)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.retract(Short.MIN_VALUE, (short) 1)) + .isInstanceOf(ArithmeticException.class); + } + + @Test + public void testFieldSumIntOverflow() { + FieldSumAgg fieldSumAgg = new FieldSumAggFactory().create(new IntType(), null, null); + assertThat(fieldSumAgg.agg(1, 2)).isEqualTo(3); + assertThatThrownBy(() -> fieldSumAgg.agg(Integer.MAX_VALUE, 1)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.agg(Integer.MIN_VALUE, -1)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.retract(Integer.MIN_VALUE, 1)) + .isInstanceOf(ArithmeticException.class); + } + + @Test + public void testFieldSumLongOverflow() { + FieldSumAgg fieldSumAgg = new FieldSumAggFactory().create(new BigIntType(), null, null); + assertThat(fieldSumAgg.agg(1L, 2L)).isEqualTo(3L); + assertThatThrownBy(() -> fieldSumAgg.agg(Long.MAX_VALUE, 1L)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.agg(Long.MIN_VALUE, -1L)) + .isInstanceOf(ArithmeticException.class); + assertThatThrownBy(() -> fieldSumAgg.retract(Long.MIN_VALUE, 1L)) + .isInstanceOf(ArithmeticException.class); + } + @Test public void testFieldProductDecimalAgg() { FieldProductAgg fieldProductAgg = From 8398d265fb6ba146a0ee5c25fd35f4344d08d8a2 Mon Sep 17 00:00:00 2001 From: dubin555 Date: Fri, 13 Mar 2026 00:22:33 +0000 Subject: [PATCH 2/2] [core] Consolidate overflow helpers into toExactByte/toExactShort Reduce four add/subtractExact{Byte,Short} methods to two toExact{Byte,Short}(int) bounds-checking helpers that take an already-computed int result, removing duplicated overflow logic. Signed-off-by: dubin555 Signed-off-by: Contributor --- .../compact/aggregate/FieldSumAgg.java | 34 +++++-------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java b/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java index 2408b7203566..cc585d6e6de0 100644 --- a/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java +++ b/paimon-core/src/main/java/org/apache/paimon/mergetree/compact/aggregate/FieldSumAgg.java @@ -55,10 +55,10 @@ public Object agg(Object accumulator, Object inputField) { mergeFieldDD.scale()); break; case TINYINT: - sum = addExactByte((byte) accumulator, (byte) inputField); + sum = toExactByte((byte) accumulator + (byte) inputField); break; case SMALLINT: - sum = addExactShort((short) accumulator, (short) inputField); + sum = toExactShort((short) accumulator + (short) inputField); break; case INTEGER: sum = Math.addExact((int) accumulator, (int) inputField); @@ -105,10 +105,10 @@ public Object retract(Object accumulator, Object inputField) { mergeFieldDD.scale()); break; case TINYINT: - sum = subtractExactByte((byte) accumulator, (byte) inputField); + sum = toExactByte((byte) accumulator - (byte) inputField); break; case SMALLINT: - sum = subtractExactShort((short) accumulator, (short) inputField); + sum = toExactShort((short) accumulator - (short) inputField); break; case INTEGER: sum = Math.subtractExact((int) accumulator, (int) inputField); @@ -142,9 +142,9 @@ private Object negative(Object value) { return Decimal.fromBigDecimal( decimal.toBigDecimal().negate(), decimal.precision(), decimal.scale()); case TINYINT: - return subtractExactByte((byte) 0, (byte) value); + return toExactByte(-(byte) value); case SMALLINT: - return subtractExactShort((short) 0, (short) value); + return toExactShort(-(short) value); case INTEGER: return Math.negateExact((int) value); case BIGINT: @@ -162,32 +162,14 @@ private Object negative(Object value) { } } - private static byte addExactByte(byte a, byte b) { - int result = a + b; + private static byte toExactByte(int result) { if (result > Byte.MAX_VALUE || result < Byte.MIN_VALUE) { throw new ArithmeticException("byte overflow"); } return (byte) result; } - private static short addExactShort(short a, short b) { - int result = a + b; - if (result > Short.MAX_VALUE || result < Short.MIN_VALUE) { - throw new ArithmeticException("short overflow"); - } - return (short) result; - } - - private static byte subtractExactByte(byte a, byte b) { - int result = a - b; - if (result > Byte.MAX_VALUE || result < Byte.MIN_VALUE) { - throw new ArithmeticException("byte overflow"); - } - return (byte) result; - } - - private static short subtractExactShort(short a, short b) { - int result = a - b; + private static short toExactShort(int result) { if (result > Short.MAX_VALUE || result < Short.MIN_VALUE) { throw new ArithmeticException("short overflow"); }