You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tmgodinho (via GitHub)" <gi...@apache.org> on 2026/02/19 17:23:24 UTC
[PR] IGNITE-27739 [ignite-3]
tmgodinho opened a new pull request, #7639:
URL: https://github.com/apache/ignite-3/pull/7639
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 Fix decimal precision validation [ignite-3]
Posted by "tmgodinho (via GitHub)" <gi...@apache.org>.
tmgodinho commented on code in PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639#discussion_r2832982901
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java:
##########
@@ -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;
Review Comment:
I don't think so, we have:
https://github.com/apache/ignite-3/blob/579169624e2fb211d2e9469e6a591169579cf679/modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java#L219-L221 before the bound validations.
Moreover, the other bound validations are also casting explicitly:
https://github.com/apache/ignite-3/blob/579169624e2fb211d2e9469e6a591169579cf679/modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java#L236-L244
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 Fix decimal precision validation [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639#discussion_r2832814787
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java:
##########
@@ -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;
Review Comment:
Can this cast fail in some case?
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/RowAssembler.java:
##########
@@ -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);
Review Comment:
Was there a test for this removed behavior that still passes?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 Fix decimal precision validation [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 [ignite-3]
Posted by "tmgodinho (via GitHub)" <gi...@apache.org>.
tmgodinho commented on PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639#issuecomment-3928717609
This should also fix: https://issues.apache.org/jira/browse/IGNITE-22965
However, there is another TODO related to that ticket:
https://github.com/apache/ignite-3/blob/084af6c3343e43d1d91de1eb5ad53ce08e2c2a57/modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java#L249-L266
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.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 Fix decimal precision validation [ignite-3]
Posted by "tmgodinho (via GitHub)" <gi...@apache.org>.
tmgodinho commented on code in PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639#discussion_r2833016062
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/row/RowAssembler.java:
##########
@@ -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);
Review Comment:
Good question. Yes. The actual tests that are checking for the exceptions are:
https://github.com/apache/ignite-3/blob/084af6c3343e43d1d91de1eb5ad53ce08e2c2a57/modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java#L354-L383
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:
https://github.com/apache/ignite-3/blob/084af6c3343e43d1d91de1eb5ad53ce08e2c2a57/modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientMarshallingTest.java#L340-L352
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 (https://github.com/apache/ignite-3/blob/8b8d67c6710847614443d29e2655cdfb605e300b/modules/table/src/main/java/org/apache/ignite/internal/schema/marshaller/TupleMarshallerImpl.java#L226-L248) method. This definitely flexes a bit for whoever is interested in finding the validation method. Nonetheless, it was already that way before.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27739 Fix decimal precision validation [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7639:
URL: https://github.com/apache/ignite-3/pull/7639#discussion_r2832837821
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/Column.java:
##########
@@ -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) {
Review Comment:
`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.
```suggestion
} 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);
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org