IGNITE-27739 Fix decimal precision validation

[tmgodinho]

https://issues.apache.org/jira/browse/IGNITE-27739

What was done:

• Added step in column to validate Decimals
• Moved validation from the RowAssembler class to Column. Therefore, the exception is triggered on validation.

[tmgodinho]

This should also fix: https://issues.apache.org/jira/browse/IGNITE-22965

However, there is another TODO related to that ticket:

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

Lines 249 to 266 in 084af6c

	@Test
	public void testIncompatibleTupleElementType() {
	var tableName = "testIncompatibleTupleElementType";
	ignite().sql().execute("CREATE TABLE " + tableName + " (KEY INT PRIMARY KEY, VAL VARCHAR NOT NULL)");
	Table table = ignite().tables().table(tableName);
	var tupleView = table.recordView();
	Tuple rec = Tuple.create().set("KEY", 1).set("VAL", 1L);
	// TODO: https://issues.apache.org/jira/browse/IGNITE-22965.
	// The validation done on a client side (for a thin client), and messages may differ between embedded clients and thin clients.
	// For an embedded client the message include type precision, but for a thin client it doesn't.
	MarshallerException ex = assertThrows(MarshallerException.class, () -> tupleView.upsert(null, rec));
	assertEquals(Marshalling.COMMON_ERR, ex.code());
	assertThat(ex.getMessage(), containsString("Value type does not match [column='VAL', expected=STRING"));
	assertThat(ex.getMessage(), endsWith(", actual=INT64]"));
	}

I'm not sure what we are supposed to do with that TODO: Should we make the exceptions the same in both clients??
If so, I would explicitly create a ticket saying that, update the reference, and close IGNITE-22965.

[ptupitsyn]

Can this cast fail in some case?

[tmgodinho]

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);
	}

[ptupitsyn]

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

[tmgodinho]

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.

[Copilot]

Pull request overview

Fixes DECIMAL precision/scale overflow validation (IGNITE-27739) by relocating the overflow check from low-level row assembly to column-level validation, and updates/extends tests to cover the corrected behavior.

Changes:

• Add DECIMAL precision validation to Column.validate(...) (triggering overflow errors during validation).
• Remove DECIMAL overflow validation from RowAssembler.appendDecimalNotNull(...).
• Update and expand tests for decimal overflow and decimal size estimation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
modules/table/src/test/java/org/apache/ignite/internal/schema/marshaller/TupleMarshallerStatisticsTest.java	Adjusts DECIMAL type parameters in the estimation test to use a valid precision/scale combination.
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/RowAssembler.java	Removes DECIMAL overflow validation previously done during row assembly.
modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java	Adds DECIMAL precision validation in validate() and removes the now-unused overflow message helper.
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java	Re-enables the existing decimal overflow test and adds another overflow regression test case.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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);
	}