Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2357] improvement(trino): Fix varchar type mapping between Gravitino JDBC catalog and Trino #2358

Merged
merged 17 commits into from
Mar 11, 2024

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Feb 26, 2024

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.

@yuqi1129 yuqi1129 marked this pull request as draft February 26, 2024 14:35
@yuqi1129 yuqi1129 marked this pull request as ready for review February 28, 2024 06:58
@yuqi1129 yuqi1129 self-assigned this Feb 28, 2024
@yuqi1129
Copy link
Contributor Author

@diqiu50
Could you please help me take a look? Thank you.

Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding integration test cases for char/varchar and testing boundary conditions.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use the instanceof operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Mar 6, 2024

@diqiu50
Could you please help me take another look?

)
COMMENT ''"

<QUERY_FAILED> Unknown type 'char(65537)' for column 'name'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error message should not be understandable by the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is from Trino itself, not from the Gravitino connector.

)
COMMENT ''"

<QUERY_FAILED> Failed to create table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we show more detailed messages for the error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to handle this special case to provide more error information, so I have omitted it.

COMMENT ''"

<QUERY_FAILED> Failed to create table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing line 25 command output, Does it still work with subsequent commands?

SHOW CREATE TABLE "test.gt_postgresql".varchar_db1.tb06;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

SHOW CREATE TABLE "test.gt_mysql".varchar_db1.tb04;

CREATE TABLE tb05 (id int, name varchar(256));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the test cases:

CREATE TABLE tb01 (id int, name char);
CREATE TABLE tb01 (id int, name varchar);

Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yuqi1129 yuqi1129 merged commit 77fcc13 into apache:main Mar 11, 2024
12 checks passed
@yuqi1129 yuqi1129 deleted the issue_2357 branch March 11, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants