[#2357] improvement(trino): Fix varchar type mapping between Gravitino JDBC catalog and Trino #2358
Conversation
|
@diqiu50 |
...va/com/datastrato/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Show resolved
Hide resolved
| if (gravitinoType.name() == Name.VARCHAR) { | ||
| if (((Types.VarCharType) gravitinoType).length() > MYSQL_CHAR_LENGTH_LIMIT) { | ||
| Class<? extends io.trino.spi.type.Type> typeClass = type.getClass(); | ||
| if (typeClass == io.trino.spi.type.CharType.class) { |
There was a problem hiding this comment.
Why don't we use the instanceof operator?
There was a problem hiding this comment.
I don't have any preference about instanceof and getClass(), as we will judge the type condition twice, so I think the the temporary variant and use the format == may be simple.
|
@diqiu50 |
| ) | ||
| COMMENT ''" | ||
|
|
||
| <QUERY_FAILED> Unknown type 'char(65537)' for column 'name' |
There was a problem hiding this comment.
I think the error message should not be understandable by the user
There was a problem hiding this comment.
The error message is from Trino itself, not from the Gravitino connector.
| ) | ||
| COMMENT ''" | ||
|
|
||
| <QUERY_FAILED> Failed to create table. |
There was a problem hiding this comment.
Why do we show more detailed messages for the error
There was a problem hiding this comment.
I need to handle this special case to provide more error information, so I have omitted it.
| COMMENT ''" | ||
|
|
||
| <QUERY_FAILED> Failed to create table. | ||
|
|
There was a problem hiding this comment.
Missing line 25 command output, Does it still work with subsequent commands?
SHOW CREATE TABLE "test.gt_postgresql".varchar_db1.tb06;
| SHOW CREATE TABLE "test.gt_mysql".varchar_db1.tb04; | ||
|
|
||
| CREATE TABLE tb05 (id int, name varchar(256)); | ||
|
|
There was a problem hiding this comment.
Missing the test cases:
CREATE TABLE tb01 (id int, name char);
CREATE TABLE tb01 (id int, name varchar);
What changes were proposed in this pull request?
Modify the type mapping for VARCHAR and CHAR to match Gravitino and Trino.
Why are the changes needed?
please see issue #2356
Fix: #2035
Fix: #2357
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
New UTs.