[#2571] feat(catalog-jdbc-doris): Add database operations for Doris catalog #2734
Conversation
zhoukangcn
force-pushed
the
f-2571
branch
2 times, most recently
from
62d8eb5 to
ec8ea7f
Compare
.../src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisExceptionConverter.java
Show resolved
Hide resolved
.../src/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisDatabaseOperations.java
Outdated
Show resolved
Hide resolved
| return sqlBuilder.toString(); | ||
| } | ||
|
|
||
| try (final Connection connection = this.dataSource.getConnection()) { |
There was a problem hiding this comment.
Can we only use one try statement here? It seems excessive.
| throw new UnsupportedOperationException(); | ||
| public JdbcSchema load(String databaseName) throws NoSuchSchemaException { | ||
| try (final Connection connection = this.dataSource.getConnection()) { | ||
| String query = "SELECT * FROM information_schema.SCHEMATA WHERE SCHEMA_NAME = ?"; |
There was a problem hiding this comment.
We do not suggest using system table information_schema.SCHEMATA, please refer to method JdbcTableOperations#load
There was a problem hiding this comment.
Maybe we can only use information_schema.SCHEMATA for database information.
There was a problem hiding this comment.
@zhoukangcn
We are going to remove all places that depend on system table like #2616
There was a problem hiding this comment.
I see. I will update my code when 2616 is merged
zhoukangcn
force-pushed
the
f-2571
branch
6 times, most recently
from
ae0dfad to
ec32572
Compare
zhoukangcn
force-pushed
the
f-2571
branch
2 times, most recently
from
86fce6c to
a76ad2a
Compare
| String databaseName, String comment, Map<String, String> properties) { | ||
| if (StringUtils.isNotEmpty(comment)) { | ||
| throw new UnsupportedOperationException( | ||
| "Doris doesn't support set database comment: " + comment); |
There was a problem hiding this comment.
Since Doris supports database properties, should we store the comment in the properties instead of throwing an exception?
There was a problem hiding this comment.
BTW, the gravitino.identifier was stored in the comment, you should extract it and put it into the properties.
There was a problem hiding this comment.
Got it, I will update to support comment and gravitino.identifier
There was a problem hiding this comment.
@mchades I had update code, store comment in database properties.
In my opinion, it is sufficient to store comments in the attribute with gravitino.identifier. There is no need to extract it. Do you have any suggestions?
|
|
||
| // Doris does not support setting schema comment, put comment in properties | ||
| Map<String, String> newProperties = new HashMap<>(properties); | ||
| if (StringUtils.isNotEmpty(comment)) { |
There was a problem hiding this comment.
Thanks for remind. this line has been removed.
| @SuppressWarnings("FormatStringAnnotation") | ||
| @Override | ||
| public GravitinoRuntimeException toGravitinoException(SQLException se) { | ||
| // TODO: add implementation for doris catalog | ||
| int errorCode = se.getErrorCode(); |
There was a problem hiding this comment.
cc @xloya For relational storage backend, could we add the similar logic?
There was a problem hiding this comment.
I think it's OK, but we may need to add the database type in the error code processing logic, because the error codes defined by different databases may be different for one error. Currently, we could only consider about MySQL.
There was a problem hiding this comment.
Maybe you can define a similar interface.
What changes were proposed in this pull request?
Add database operations for Doris catalog
Why are the changes needed?
Fix: #2571
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
UT & IT