[CALCITE-6699] Invalid unparse for Varchar in StarRocksDialect #4123
Conversation
core/src/main/java/org/apache/calcite/sql/dialect/StarRocksSqlDialect.java
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
Show resolved
Hide resolved
xiaochen-zhou
force-pushed
the
sr_dialect_varchar
branch
from
ece76dc to
41d4e9f
Compare
|
Hi @xiaochen-zhou , you don't need to force-push for every commit, and it helps other reviewers to see the changes better. |
OK. Thank you for your reminder. |
core/src/main/java/org/apache/calcite/sql/dialect/StarRocksSqlDialect.java
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/sql/dialect/StarRocksSqlDialect.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
Outdated
Show resolved
Hide resolved
| @Test void testStarRocksCastToVarcharWithGreaterThanMaxPrecision() { | ||
| final String query = "select cast(\"product_id\" as varchar(150000)), \"product_id\" " | ||
| + "from \"product\" "; | ||
| final String expected = "SELECT CAST(`product_id` AS VARCHAR(65533)), `product_id`\n" |
There was a problem hiding this comment.
is this supposed to change the precision silently, or should this at least produce a warning?
There was a problem hiding this comment.
@mihaibudiu I noticed that our default implementation silently changes the conversion in SQLTypeUtil.convertTypeToSpec, so I think there is no problem here.
|
I have updated the code. Please take a look when you have time. @mihaibudiu @NobiGo @ILuffZhe |
| @@ -128,6 +153,10 @@ public StarRocksSqlDialect(Context context) { | |||
| type.getSqlTypeName(), | |||
| SqlParserPos.ZERO), | |||
| SqlParserPos.ZERO); | |||
| case VARCHAR: | |||
There was a problem hiding this comment.
I remember we discussed this issue before. Does StarRocks have any restrictions on varchar types, such as varchar(65533)? If so, please explain it, at least add JavaDoc.
There was a problem hiding this comment.
I remember we discussed this issue before. Does StarRocks have any restrictions on varchar types, such as varchar(65533)? If so, please explain it, at least add JavaDoc.
There are restrictions.
Calcite will truncate according to the maximum precision. I have added test cases
| public static final RelDataTypeSystem STARROCKS_TYPE_SYSTEM = | ||
| new RelDataTypeSystemImpl() { | ||
| @Override public int getMaxPrecision(SqlTypeName typeName) { | ||
| switch (typeName) { |
| @@ -3747,6 +3747,30 @@ private SqlDialect nonOrdinalDialect() { | |||
| sql(query).dialect(mySqlDialect(NullCollation.LAST)).ok(expected); | |||
| } | |||
|
|
|||
| @Test void testStarRocksCastToVarcharWithLessThanMaxPrecision() { | |||
There was a problem hiding this comment.
Add jira link
Done.
mihaibudiu
added
the
LGTM-will-merge-soon
xiaochen-zhou
force-pushed
the
sr_dialect_varchar
branch
from
c0b92bf to
fb7d844
Compare
xiaochen-zhou
force-pushed
the
sr_dialect_varchar
branch
from
fb7d844 to
d60676d
Compare
|
|
I have updated the code. Please take a look when you have time. @mihaibudiu @NobiGo @ILuffZhe @caicancai |
When performing starrocks dialect conversion for varchar, it defaults to using MySQL dialect，MySQL doesn't have a VARCHAR type, only CHAR,So during StarRocks dialect conversion, VARCHAR will also be converted to CHAR
Since StarRocks supports the VARCHAR type, we should modify the StarRocks dialect
https://docs.starrocks.io/docs/sql-reference/data-types/string-type/VARCHAR/