-
Notifications
You must be signed in to change notification settings - Fork 392
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
[#655] feat(jdbc): Initialize the JDBC module in Gravitino. #656
Conversation
Code Coverage Report
|
b51924b
to
4fafa91
Compare
4fafa91
to
bcf3e40
Compare
@Clearvive is it ready for review? |
Yes, I have split the pull request for easier review. |
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcTable.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcTable.java
Outdated
Show resolved
Hide resolved
.../catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
Outdated
Show resolved
Hide resolved
.../catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcColumn.java
Outdated
Show resolved
Hide resolved
@FANNG1 would you please also take a review. |
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalog.java
Outdated
Show resolved
Hide resolved
ok |
...og-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcTablePropertiesMetadata.java
Outdated
Show resolved
Hide resolved
...g-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcSchemaPropertiesMetadata.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcColumn.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcColumn.java
Outdated
Show resolved
Hide resolved
@Clearvive can you please briefly describe how do you want to organize your code regard to jdbc catalog, say if we have common jdbc code and specific jdbc implementations (MySQL)? I think we're not in the same page, it would be better if you can briefly describe it here. |
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcColumn.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcSchema.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcTable.java
Outdated
Show resolved
Hide resolved
c14200c
to
eaa6225
Compare
...ommon/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java
Outdated
Show resolved
Hide resolved
@Clearvive please reply this. |
...common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcSchemaPropertiesMetadata.java
Outdated
Show resolved
Hide resolved
catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcTable.java
Outdated
Show resolved
Hide resolved
If we are dealing with MySQL, there will be a Additionally, the corresponding directory structure for our modules is as follows:
|
I remembered that @mchades brought out a discussion about whether we should use different catalogs (shortName) to represent different jdbc sources, or we use a unified jdbc catalog with different types (MySQL, PostgreSQL...). One thing I am concerned about is that if we have many jdbc databases supported, we may end up with many different implementations, and potentially many duplicated codes. So what is your thought? |
Offline talked to @mchades @Clearvive , it's hard to just change a type to reuse existing Jdbc-connectors like MySQL, because Jdbc is mainly for commucatition. Every engine had it's specify grammar, properties, type systems. see more in #651 |
OK, I see. |
@FANNG1 can you please help to review again. |
LGTM |
What changes were proposed in this pull request?
Regarding JDBC as a catalog, we should complete its design on how to use it in Gravitino.
Why are the changes needed?
Fix: #573
Does this PR introduce any user-facing change?
No
How was this patch tested?
No