IGNITE-19893: Sql. Add plan checks to BaseIndexDataTypeTest. #2422
Conversation
...onTest/java/org/apache/ignite/internal/sql/engine/datatypes/tests/BaseIndexDataTypeTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| // No need to binary between char and varbinary. | ||
| if (SqlTypeUtil.isBinary(toType) && SqlTypeUtil.isBinary(fromType)) { |
There was a problem hiding this comment.
this check is not operate with char types at all, why char is mentioned here ? also "No need to binary" is sounds strange, isn`t it ?
There was a problem hiding this comment.
That was a typo, thanks.
There was a problem hiding this comment.
I still think that such kind of fix is looks like a stub, varchar(default_precision) and varchar need to be equal types is not it ?
There was a problem hiding this comment.
What kind of fix are we talking about - you mean rewriting logic related to creating reldata types with default precision/scale?
Add missing check.
fix style issues.
Add comments that clarifies checks for binary/char types.
Add comments that clarifies checks for binary/char types.
Add test label.
update comment.
|
Removed the remaining outdated information from javadocs of |
zstan
left a comment
zstan
left a comment
There was a problem hiding this comment.
overall looks good, instead of still possible cast from char to varbinary (check comments) probably you can fix it ?
| * <ul> | ||
| * <li>{@code <type>} - an SQL name of a data type.</li> | ||
| * <li>{@code $N} - the {@code N-th} value from sample values (0-based), converted to an SQL expression by | ||
| * {@link DataTypeTestSpec#toValueExpr(Comparable)} call (0-based)</li> |
There was a problem hiding this comment.
not always converted by DataTypeTestSpec#toValueExpr i suppose ? but after change :
if (useLiterals) {
placeHolderValue = testTypeSpec.toLiteral(value);
} else {
placeHolderValue = testTypeSpec.toValueExpr(value);
}
toLiteral - is used ?
| case LITERAL: | ||
| return spec.toLiteral(value); | ||
| case CAST: { | ||
| String str = spec.toStringValue(value); |
There was a problem hiding this comment.
https://issues.apache.org/jira/browse/IGNITE-20185 will reject casting from char to varbinary i.e. '1'::varbinary would be rejected, also it`s not allowed by standard. But i also understand that this is without the scope of this issue but if you can easily fix it - it would be great.
There was a problem hiding this comment.
Replaced string literals with binary literals.
Fix CASTs - use casts from VARBINARY literal instead of CHAR literals.
|
|
||
| @BeforeAll | ||
| public void createTable() { | ||
| runSql("CREATE TABLE varbinary_fixed_length(id INTEGER PRIMARY KEY, test_key VARBINARY(10))"); |
There was a problem hiding this comment.
VARBINARY is variable length BINARY type.
What does varbinary_fixed_length mean?
There was a problem hiding this comment.
If you mean we could create columns as
col1 VARBINARY,
col2 VARBINARY(10)
then, actually, col1 will have a max length as well.
We always create VARBINARY/VARCHAR column with specific length, which is 65k by default as I remember.
There was a problem hiding this comment.
VARBINARY is variable length BINARY type.
What does varbinary_fixed_length mean?
Right now VARBINARY(n) works exactly the same way as does BINARY(n). If this is a bug, I can replace VARBINARY(n) with BINARY(n) here, I could file an issue to fix this.
There was a problem hiding this comment.
Removed varbinary_fixed_length as it duplicates test cases defined in base class.
Remove redundant case.
|
Unmuted |
|
@lowka can you merge fresh main plz ? |
|
@zstan Merged the latest main into this PR. |
| private static Stream<Arguments> indexChecks() { | ||
| return Stream.of( | ||
| Arguments.of(Named.of("VARBINARY_DEFAULT_LENGTH", "T"), ValueMode.LITERAL), | ||
| //Arguments.of(Named.of("VARBINARY_DEFAULT_LENGTH", "T"), ValueMode.CAST), |
…ort-based map/reduce aggregates. cleanup.
4 participants
Adds index usage checks to
BaseIndexDataTypeTest. In addition to that fixes the issue with VARBINARY, mentioned in issue's comment.https://issues.apache.org/jira/browse/IGNITE-19893
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
- 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.
Notes