Skip to content

IGNITE-27739 Fix decimal precision validation #7639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tmgodinho wants to merge 4 commits into
base: main
Choose a base branch
from
+32 −19
Open

IGNITE-27739 Fix decimal precision validation #7639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5791696
IGNITE-27739 Move decimal validation from RowAssebler to Column valid…
tmgodinho Feb 19, 2026
c9facc1
IGNITE-27739 Add bug reproducer: testDecimalColumnOverflow2
tmgodinho Feb 19, 2026
084af6c
IGNITE-27739 Remove disabled test: testDecimalColumnOverflow
tmgodinho Feb 19, 2026
942ba44
IGNITE-27739 Fix TupleMarshallerStatisticsTest#testDecimalSizeEstimation
tmgodinho Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions ...onTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.math.BigDecimal;
import java.util.Map;
import org.apache.ignite.Ignite;
import org.apache.ignite.lang.ErrorGroups.Marshalling;
import org.apache.ignite.lang.MarshallerException;
import org.apache.ignite.table.RecordView;
import org.apache.ignite.table.Table;
import org.apache.ignite.table.Tuple;
import org.apache.ignite.table.mapper.Mapper;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.function.Executable;

Expand Down Expand Up @@ -352,7 +352,6 @@ public void testVarcharColumnOverflow() {
}

@Test
@Disabled("https://issues.apache.org/jira/browse/IGNITE-22965")
public void testDecimalColumnOverflow() {
var tableName = "testDecimalColumnOverflow";

Expand All @@ -366,6 +365,23 @@ public void testDecimalColumnOverflow() {
"Numeric field overflow in column 'VAL'");
}

@Test
void testDecimalColumnOverflow2() {
var tableName = "testDecimalColumnOverflow2";

ignite().sql().execute("CREATE TABLE " + tableName + " (KEY INT PRIMARY KEY, STR VARCHAR(10), VAL DECIMAL(6,2))");

Table table = ignite().tables().table(tableName);
var view = table.keyValueView();

Tuple key = Tuple.create(Map.of("key", 1));
Tuple value = Tuple.create(Map.of("val", new BigDecimal("89900.123456")));

assertThrowsMarshallerException(
() -> view.put(key, value),
"Numeric field overflow in column 'VAL'");
}

@Test
public void testUnsupportedObjectInTuple() {
Table table = ignite().tables().table(TABLE_NAME);
Expand Down
23 changes: 13 additions & 10 deletions modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@

import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
import org.apache.ignite.internal.tostring.IgniteToStringExclude;
import org.apache.ignite.internal.tostring.S;
import org.apache.ignite.internal.type.DecimalNativeType;
import org.apache.ignite.internal.type.NativeType;
import org.apache.ignite.internal.type.NativeTypes;
import org.apache.ignite.internal.type.VarlenNativeType;
Expand Down Expand Up @@ -236,6 +239,8 @@ public void validate(@Nullable Object val) {
checkBounds((LocalDateTime) val, SchemaUtils.DATETIME_MIN, SchemaUtils.DATETIME_MAX);
} else if (type.spec() == ColumnType.TIMESTAMP) {
checkBounds((Instant) val, SchemaUtils.TIMESTAMP_MIN, SchemaUtils.TIMESTAMP_MAX);
} else if (type.spec() == ColumnType.DECIMAL) {
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate() unconditionally casts val to BigDecimal for DECIMAL columns. If an unsupported value type reaches here (i.e., NativeTypes.fromObject(val) returns null so the mismatch branch is skipped), this will throw a ClassCastException and bypass the usual marshalling error wrapping. Consider guarding the DECIMAL branch with an instanceof BigDecimal (or similar) so unsupported types fail via the existing, consistent type-mismatch path instead of a raw CCE.

Suggested change
} else if (type.spec() == ColumnType.DECIMAL) {
} else if (type.spec() == ColumnType.DECIMAL) {
if (!(val instanceof BigDecimal)) {
String actualTypeName = objType != null ? objType.displayName() : val.getClass().getSimpleName();
String error = format(
"Value type does not match [column='{}', expected={}, actual={}]",
name, type.displayName(), actualTypeName
);
throw new InvalidTypeException(error);
}

Copilot uses AI. Check for mistakes.
checkPrecision((BigDecimal) val);
}
}

Expand All @@ -246,6 +251,14 @@ private <T extends Comparable<T>> void checkBounds(T value, T min, T max) {
}
}

private void checkPrecision(BigDecimal val) throws SchemaMismatchException {
DecimalNativeType dnt = (DecimalNativeType) type;
Copy link
Contributor

@ptupitsyn ptupitsyn Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this cast fail in some case?

Copy link
Contributor Author

@tmgodinho tmgodinho Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, we have:

ignite-3/modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java

Lines 219 to 221 in 5791696

NativeType objType = NativeTypes.fromObject(val);
if (objType != null && type.mismatch(objType)) {
before the bound validations.

Moreover, the other bound validations are also casting explicitly:

ignite-3/modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java

Lines 236 to 244 in 5791696

if (type.spec() == ColumnType.DATE) {
checkBounds((LocalDate) val, SchemaUtils.DATE_MIN, SchemaUtils.DATE_MAX);
} else if (type.spec() == ColumnType.DATETIME) {
checkBounds((LocalDateTime) val, SchemaUtils.DATETIME_MIN, SchemaUtils.DATETIME_MAX);
} else if (type.spec() == ColumnType.TIMESTAMP) {
checkBounds((Instant) val, SchemaUtils.TIMESTAMP_MIN, SchemaUtils.TIMESTAMP_MAX);
} else if (type.spec() == ColumnType.DECIMAL) {
checkPrecision((BigDecimal) val);
}

BigDecimal scaled = val.setScale(dnt.scale(), RoundingMode.HALF_UP);
if (scaled.precision() > dnt.precision()) {
throw new SchemaMismatchException(format("Numeric field overflow in column '{}'", name));
}
}

/**
* Creates copy of the column with assigned positions.
*
Expand Down Expand Up @@ -281,14 +294,4 @@ public String toString() {
public static String nullConstraintViolationMessage(String columnName) {
return format("Column '{}' does not allow NULLs", columnName);
}

/**
* Returns an error message for numeric field overflow error.
*
* @param columnName Column name.
* @return Error message.
*/
public static String numericFieldOverflow(String columnName) {
return format("Numeric field overflow in column '{}'", columnName);
}
}
6 changes: 0 additions & 6 deletions modules/schema/src/main/java/org/apache/ignite/internal/schema/row/RowAssembler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package org.apache.ignite.internal.schema.row;

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
Expand Down Expand Up @@ -361,11 +360,6 @@ public RowAssembler appendDecimalNotNull(BigDecimal val) throws SchemaMismatchEx

DecimalNativeType type = (DecimalNativeType) col.type();

BigDecimal scaled = val.setScale(type.scale(), RoundingMode.HALF_UP);
Copy link
Contributor

@ptupitsyn ptupitsyn Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a test for this removed behavior that still passes?

Copy link
Contributor Author

@tmgodinho tmgodinho Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Yes. The actual tests that are checking for the exceptions are:

ignite-3/modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java

Lines 354 to 383 in 084af6c

@Test
public void testDecimalColumnOverflow() {
var tableName = "testDecimalColumnOverflow";
ignite().sql().execute("CREATE TABLE " + tableName + " (KEY INT PRIMARY KEY, VAL DECIMAL(3,1))");
Table table = ignite().tables().table(tableName);
var tupleView = table.keyValueView();
assertThrowsMarshallerException(
() -> tupleView.put(null, Tuple.create().set("KEY", 1), Tuple.create().set("VAL", new BigDecimal("12345.1"))),
"Numeric field overflow in column 'VAL'");
}
@Test
void testDecimalColumnOverflow2() {
var tableName = "testDecimalColumnOverflow2";
ignite().sql().execute("CREATE TABLE " + tableName + " (KEY INT PRIMARY KEY, STR VARCHAR(10), VAL DECIMAL(6,2))");
Table table = ignite().tables().table(tableName);
var view = table.keyValueView();
Tuple key = Tuple.create(Map.of("key", 1));
Tuple value = Tuple.create(Map.of("val", new BigDecimal("89900.123456")));
assertThrowsMarshallerException(
() -> view.put(key, value),
"Numeric field overflow in column 'VAL'");
}

Essentially, before the PR, the validation for decimals was done here. After the PR, the validation is now done on Column#validate() with seemed to me more similar to the other checks. For instance:

ignite-3/modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java

Lines 340 to 352 in 084af6c

@Test
public void testVarcharColumnOverflow() {
var tableName = "testVarcharColumnOverflow";
ignite().sql().execute("CREATE TABLE " + tableName + " (KEY INT PRIMARY KEY, VAL VARCHAR(10))");
Table table = ignite().tables().table(tableName);
var tupleView = table.keyValueView();
assertThrowsMarshallerException(
() -> tupleView.put(null, Tuple.create().set("KEY", 1), Tuple.create().set("VAL", "1".repeat(20))),
"Value too long [column='VAL', type=STRING(10)]");
}

I also did not find much validation code on row assembler besides checkNull and checkType and this decimals stuff. That's why I removed the check here.

The only smell I found is that the validation itself is now done under the gatherStatistics (

ignite-3/modules/table/src/main/java/org/apache/ignite/internal/schema/marshaller/TupleMarshallerImpl.java

Lines 226 to 248 in 8b8d67c

void gatherStatistics(
TuplePart part,
Tuple tuple,
ValuesWithStatistics targetTuple
) throws SchemaMismatchException {
int estimatedValueSize = part.fixedSizeColumnsSize(keyOnlyFixedLengthColumnSize, valueOnlyFixedLengthColumnSize);
int knownColumns = 0;
for (Column col : part.deriveColumnList(schema)) {
NativeType colType = col.type();
Object val = TupleHelper.valueOrDefault(tuple, col.name(), POISON_OBJECT);
if (val == POISON_OBJECT && col.positionInKey() != -1) {
throw new SchemaMismatchException("Missed key column: " + col.name());
}
if (val == POISON_OBJECT) {
val = col.defaultValue();
} else {
knownColumns++;
}
col.validate(val);
) method. This definitely flexes a bit for whoever is interested in finding the validation method. Nonetheless, it was already that way before.

if (scaled.precision() > type.precision()) {
throw new SchemaMismatchException(Column.numericFieldOverflow(col.name()));
}

builder.appendDecimalNotNull(val, type.scale());

shiftColumn();
Expand Down
2 changes: 1 addition & 1 deletion ...test/java/org/apache/ignite/internal/schema/marshaller/TupleMarshallerStatisticsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public class TupleMarshallerStatisticsTest {
@ValueSource(ints = {3, 1024, CatalogUtils.MAX_DECIMAL_SCALE - PRECISION})
public void testDecimalSizeEstimation(int columnScale) {
SchemaDescriptor schema = new SchemaDescriptor(1,
new Column[]{new Column("KEY", NativeTypes.decimalOf(PRECISION, columnScale), false)},
new Column[]{new Column("KEY", NativeTypes.decimalOf(PRECISION + columnScale, columnScale), false)},
new Column[]{new Column("UNUSED", NativeTypes.INT32, true)});

TupleMarshallerImpl marshaller = KeyValueTestUtils.createMarshaller(schema);
Expand Down