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

[#2572] feat(catalog-jdbc-doris): support table operation for Doris catalog #2875

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

zhoukangcn
Copy link
Contributor

What changes were proposed in this pull request?

support doris table operation

Why are the changes needed?

Fix: #2572

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

UT

@zhoukangcn zhoukangcn changed the title [#2572] feat(catalog-jdbc-doris): support doris table operation [#2572] feat(catalog-jdbc-doris): support table operation for Doris catalog Apr 10, 2024
@zhoukangcn zhoukangcn force-pushed the f-2572 branch 3 times, most recently from ef4c770 to 1c26f1e Compare April 11, 2024 00:45
@zhoukangcn zhoukangcn closed this Apr 11, 2024
@zhoukangcn zhoukangcn reopened this Apr 11, 2024
@zhoukangcn zhoukangcn closed this Apr 11, 2024
@zhoukangcn zhoukangcn reopened this Apr 11, 2024
}

@Override
protected String generatePurgeTableSql(String tableName) {
throw new UnsupportedOperationException();
return String.format("TRUNCATE TABLE `%s`", tableName);
Copy link
Contributor

@yuqi1129 yuqi1129 Apr 11, 2024

Choose a reason for hiding this comment

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

TRUNCATE TABLE in Doris means dropping all data in the table and retaining the table itself, does it align with our definition of purgeTable @mchades Please help to confirm it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here should be: truncate + drop = purge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/PURGE.html

https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/DROP-TABLE.html

@mchades I have a little question, what is the exact semantics : PURGE TABLE xx or DROP TABLE xxx PURGE

Seems there is a little diffrent

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhoukangcn

  • dropTable removes both the metadata and the directory associated with the table from the file system if the table is not an external table. In case of an external table, only the associated metadata is removed.
  • purgeTable completely removes both the metadata and the directory associated with the table and skipping trash, if the table is an external table or the catalogs don't support purge table, UnsupportedOperationException is thrown.

For more, please see: https://github.com/datastrato/gravitino/blob/9128db0d2f4bc57dbfb410b51db17887e335cba4/docs/manage-relational-metadata-using-gravitino.md?plain=1#L876C1-L881C127

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thank you very much !

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 had update the code, throw a UnsupportedOperationException in purge Doris table

@yuqi1129
Copy link
Contributor

@zhoukangcn
Besides, the code in TableOperations seems to resemble that in the MySQL catalog. Can we extract common logic and reduce the code?

@yuqi1129 yuqi1129 closed this Apr 11, 2024
@yuqi1129 yuqi1129 reopened this Apr 11, 2024
@zhoukangcn zhoukangcn force-pushed the f-2572 branch 3 times, most recently from 1f9a417 to 07291f7 Compare April 11, 2024 08:23
@zhoukangcn zhoukangcn closed this Apr 11, 2024
@zhoukangcn zhoukangcn reopened this Apr 11, 2024
@zhoukangcn zhoukangcn closed this Apr 11, 2024
@zhoukangcn zhoukangcn reopened this Apr 11, 2024
@zhoukangcn zhoukangcn force-pushed the f-2572 branch 6 times, most recently from b833b8d to 3bbbb24 Compare April 11, 2024 17:08
@zhoukangcn zhoukangcn force-pushed the f-2572 branch 4 times, most recently from f635302 to 6f6a8a4 Compare April 13, 2024 12:55
@zhoukangcn zhoukangcn force-pushed the f-2572 branch 5 times, most recently from 83de756 to 1d29b15 Compare April 15, 2024 09:48
@yuqi1129
Copy link
Contributor

@FANNG1
Do you have any further comments? It's okay with me at the moment.

preparedStatement.setString(2, tableName);

try (ResultSet resultSet = preparedStatement.executeQuery()) {
while (resultSet.next()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to retrive multi comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly resultSet just returns one line, but I think it's necessary to iterate through the resultSet.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bug if geting more than one line?

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 can't temporarily construct a multi-line test case, but using a while loop here to ensure all resultSets are read seems to pose no problem.

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 16, 2024

LGTM except one minor comment.

Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM, besides, about Index, distribution, partitioning, do forget to create related issues.

@FANNG1 FANNG1 merged commit 6eccc74 into apache:main Apr 16, 2024
22 checks passed
@FANNG1
Copy link
Contributor

FANNG1 commented Apr 16, 2024

@zhoukangcn merged to main, thanks for your work!

@zhoukangcn zhoukangcn deleted the f-2572 branch April 16, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask] add table operations for Doris catalog
4 participants