You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korlov42 (via GitHub)" <gi...@apache.org> on 2024/10/15 12:10:00 UTC
[PR] IGNITE-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
korlov42 opened a new pull request, #4566:
URL: https://github.com/apache/ignite-3/pull/4566
https://issues.apache.org/jira/browse/IGNITE-23172
This patch changes the rules for type derivation of dynamic param for BigDecimal:
- by default type is DECIMAL(28, 6)
- there is an ability to specify precise type by wrapping dye param placeholder with CAST operator
---------
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566#discussion_r1802404655
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -92,6 +96,45 @@ void testMetadataTypesForDynamicParameters(ColumnType type) {
assertQuery("SELECT ?").withParams(param).returns(param).columnMetadata(new MetadataMatcher().type(type)).check();
}
+ /**
+ * By default derived type of parameter of type BigDecimal is DECIMAL(28, 6). This test makes sure it's possible to go beyond
+ * default precision and scale by wrapping dynamic param placeholder with CAST operation.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {30, 60, 120})
Review Comment:
done
--
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566#discussion_r1801342639
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -92,6 +96,45 @@ void testMetadataTypesForDynamicParameters(ColumnType type) {
assertQuery("SELECT ?").withParams(param).returns(param).columnMetadata(new MetadataMatcher().type(type)).check();
}
+ /**
+ * By default derived type of parameter of type BigDecimal is DECIMAL(28, 6). This test makes sure it's possible to go beyond
+ * default precision and scale by wrapping dynamic param placeholder with CAST operation.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {30, 60, 120})
Review Comment:
let's also check values less than default, like a 10, 20
--
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566
--
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566#discussion_r1803038245
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -81,9 +82,12 @@ void testMetadataTypesForDynamicParameters(ColumnType type) {
// TODO https://issues.apache.org/jira/browse/IGNITE-19162 Ignite SQL doesn't support precision more than 3 for temporal types.
if (type == ColumnType.TIME || type == ColumnType.TIMESTAMP || type == ColumnType.DATETIME) {
param = SqlTestUtils.generateValueByType(type, 3, -1);
+ } else if (type == ColumnType.DECIMAL) { // by default derived type of parameter of type BigDecimal is DECIMAL(28, 6)
+ param = SqlTestUtils.generateValueByType(type, 28, 6);
Review Comment:
why not use constants?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/NumericCaseTypeCoercionTest.java:
##########
@@ -51,7 +50,7 @@
public class NumericCaseTypeCoercionTest extends BaseTypeCoercionTest {
private static final IgniteSchema SCHEMA = createSchemaWithTwoColumnTable(NativeTypes.STRING, NativeTypes.STRING);
- private static final NativeType DECIMAL_MAX_PREC = NativeTypes.decimalOf(MAX_DECIMAL_PRECISION, 0);
+ private static final NativeType DECIMAL_DYN_PARAM_DEFAULT = NativeTypes.decimalOf(28, 6);
Review Comment:
why not use `DECIMAL_DYNAMIC_PARAM_PRECISION` and `DECIMAL_DYNAMIC_PARAM_SCALE` constants?
--
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566#discussion_r1803035393
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/NumericCaseTypeCoercionTest.java:
##########
@@ -51,7 +50,7 @@
public class NumericCaseTypeCoercionTest extends BaseTypeCoercionTest {
private static final IgniteSchema SCHEMA = createSchemaWithTwoColumnTable(NativeTypes.STRING, NativeTypes.STRING);
- private static final NativeType DECIMAL_MAX_PREC = NativeTypes.decimalOf(MAX_DECIMAL_PRECISION, 0);
+ private static final NativeType DECIMAL_DYN_PARAM_DEFAULT = NativeTypes.decimalOf(28, 6);
Review Comment:
why not use `DECIMAL_DYNAMIC_PARAM_PRECISION` and `DECIMAL_DYNAMIC_PARAM_SCALE` constants?
I mean
```
NativeTypes.decimalOf(DECIMAL_DYNAMIC_PARAM_PRECISION, DECIMAL_DYNAMIC_PARAM_SCALE);
```
--
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-23172 Sql. Lack of bounds validation in CAST(1 as DECIMAL(MAX_PREC, MAX_SCALE)) [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #4566:
URL: https://github.com/apache/ignite-3/pull/4566#discussion_r1803104980
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/NumericCaseTypeCoercionTest.java:
##########
@@ -51,7 +50,7 @@
public class NumericCaseTypeCoercionTest extends BaseTypeCoercionTest {
private static final IgniteSchema SCHEMA = createSchemaWithTwoColumnTable(NativeTypes.STRING, NativeTypes.STRING);
- private static final NativeType DECIMAL_MAX_PREC = NativeTypes.decimalOf(MAX_DECIMAL_PRECISION, 0);
+ private static final NativeType DECIMAL_DYN_PARAM_DEFAULT = NativeTypes.decimalOf(28, 6);
Review Comment:
good catch, fixed
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java:
##########
@@ -81,9 +82,12 @@ void testMetadataTypesForDynamicParameters(ColumnType type) {
// TODO https://issues.apache.org/jira/browse/IGNITE-19162 Ignite SQL doesn't support precision more than 3 for temporal types.
if (type == ColumnType.TIME || type == ColumnType.TIMESTAMP || type == ColumnType.DATETIME) {
param = SqlTestUtils.generateValueByType(type, 3, -1);
+ } else if (type == ColumnType.DECIMAL) { // by default derived type of parameter of type BigDecimal is DECIMAL(28, 6)
+ param = SqlTestUtils.generateValueByType(type, 28, 6);
Review Comment:
fixed
--
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