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.

Sign up for GitHub

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

Jump to bottom

[#777] feat(jdbc-mysql): Support for mysql catalog in Gravitino. #786

Merged
merged 3 commits into from
Nov 30, 2023
Merged

[#777] feat(jdbc-mysql): Support for mysql catalog in Gravitino. #786

merged 3 commits into from
Nov 30, 2023

Conversation

Clearvive
Copy link
Contributor

What changes were proposed in this pull request?

We need to support MySQL as the storage protocol for the catalog in Gravetino

Why are the changes needed?

Fix: #777

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Will be completed during integration testing

@Clearvive Clearvive added this to the Gravitino 0.3.0 milestone Nov 21, 2023
@Clearvive Clearvive self-assigned this Nov 21, 2023
@Clearvive Clearvive force-pushed the 777/add-mysql-catalog branch 2 times, most recently from 76264dc to 957c2a8 Compare November 22, 2023 08:56
@Clearvive
Copy link
Contributor Author

@jerryshao The code for the MySQL catalog section can now be reviewed, and the final commit can be reviewed.

@jerryshao jerryshao removed this from the Gravitino 0.3.0 milestone Nov 23, 2023
@jerryshao
Copy link
Contributor

You need to rebase the code.

@Clearvive Clearvive marked this pull request as ready for review November 24, 2023 05:08
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 24, 2023 •
edited
Loading

Code Coverage Report

Overall Project 66.05% -0.14% 🟢
Files changed 37.5% 🔴

Module Coverage
api 62.97% -0.17% 🔴
catalog-jdbc-common 41.38% -3.98% 🔴
Files
Module File Coverage
api NoSuchColumnException.java 0% 🔴
catalog-jdbc-common JdbcConnectorUtils.java 100% 🟢
JdbcTypeConverter.java 100% 🟢
JdbcColumn.java 85.71% -14.29% 🔴
JdbcTableOperations.java 78.24% -0.78% 🟢
JdbcDatabaseOperations.java 12.61% -34.23% 🔴
JdbcCatalogOperations.java 0% -2.14% 🔴

yuqi1129
yuqi1129 reviewed Nov 25, 2023
View reviewed changes
.../src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlExceptionConverter.java Show resolved Hide resolved
...mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java Outdated Show resolved Hide resolved
.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java Outdated Show resolved Hide resolved
...mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java Outdated Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Outdated Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Outdated Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Outdated Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Show resolved Hide resolved
FANNG1
FANNG1 reviewed Nov 27, 2023
View reviewed changes
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
setProperties.add(((TableChange.SetProperty) change));
} else if (change instanceof TableChange.RemoveProperty) {
// mysql does not support deleting table attributes, it can be replaced by Set Property
throw new UnsupportedOperationException("Remove property is not supported yet");
Copy link
Contributor

Choose a reason for hiding this comment

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

can we transform RemoveProperty to SetProperty in mysql? Gravitino provides a unify interface to user, I think we should do the transformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a mapping relationship, but MySQL itself does not support deleting attribute values. We believe that we should try to maintain the same semantics as MySQL as much as possible

@FANNG1
Copy link
Contributor

FANNG1 commented Nov 28, 2023

Generally LGTM, except the above comment.
@Clearvive could you describe how table and column properties are processed from the user's perspective?

@Clearvive
Copy link
Contributor Author

Generally LGTM, except the above comment. @Clearvive could you describe how table and column properties are processed from the user's perspective?

#832 I have initiated an issue, and after completing the jdbc related tasks, I will continue with this issue and ultimately write user documentation

@Clearvive Clearvive force-pushed the 777/add-mysql-catalog branch 4 times, most recently from 565550a to e63b7a3 Compare November 28, 2023 11:21
@Clearvive Clearvive added this to the Gravitino 0.3.0 milestone Nov 29, 2023
jerryshao
jerryshao reviewed Nov 29, 2023
View reviewed changes
...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java Show resolved Hide resolved
catalogs/catalog-jdbc-mysql/build.gradle.kts Show resolved Hide resolved
...gs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/MysqlCatalog.java Outdated Show resolved Hide resolved
.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java Show resolved Hide resolved
.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java Outdated Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java Show resolved Hide resolved
...logs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcColumn.java Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
if (StringUtils.isNotEmpty(column.comment())) {
sqlBuilder.append("COMMENT '").append(column.comment()).append("' ");
}
return sqlBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you support specifying column properties key index, primary key, auto increment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added field attributes in line 497, and these attributes will be added

...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
}
String col = updateColumnComment.fieldNames()[0];
JdbcColumn column = getJdbcColumnFromCreateTable(createTable, col);
column.getProperties().remove(PRIMARY_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the meaning of this 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.

In the syntax of "MODIFY COLUMN", the attribute "PRIMARY KEY" is not supported. Here, only the annotation of the column is modified, so removing this attribute to avoid errors will not remove the "PRIMARY KEY" attribute of the field

...tastrato/gravitino/integration/test/catalog/mysql/operation/TestMysqlDatabaseOperations.java
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class TestMysqlDatabaseOperations extends TestMysqlAbstractIT {
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 you should have E2E test here instead of these UTs. If you have another PR you can do it separately. Otherwise you'd do it here.

jerryshao
jerryshao reviewed Nov 29, 2023
View reviewed changes
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
* ["ENGINE","=","InnoDB","DEFAULT","CHARSET","=","utf8mb4"]
* @return key-value pairs. For example: {"ENGINE":"InnoDB","DEFAULT CHARSET":"utf8mb4"}
*/
private static Map<String, String> parseOrderedKeyValuePairs(String[] input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also add some properties as reserved properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be available in #804 Do these things

@jerryshao
Copy link
Contributor

Overall, I think the properties part (DB, table, column) doesn't have a clear definition and spec for Gravitino, mostly it directly get from underlying database and put into the properties, which is not so clear here.

My thinking is that we should have a unified definitions for all the jdbc database implementations.

@Clearvive
Copy link
Contributor Author

Clearvive commented Nov 29, 2023 •
edited
Loading

Overall, I think the properties part (DB, table, column) doesn't have a clear definition and spec for Gravitino, mostly it directly get from underlying database and put into the properties, which is not so clear here.

My thinking is that we should have a unified definitions for all the jdbc database implementations.

Yes, including in com.datastrato.gravitino.rel.TableChange.AddColumn, we also haven't defined these, so after the merge of the PostgreSql module, we will unify to add the logic of defining properties in #804.

jerryshao
jerryshao reviewed Nov 30, 2023
View reviewed changes
build.gradle.kts Outdated
@@ -366,7 +366,8 @@ tasks {

val copyCatalogLibAndConfigs by registering(Copy::class) {
dependsOn(":catalogs:catalog-hive:copyLibAndConfig",
":catalogs:catalog-lakehouse-iceberg:copyLibAndConfig")
":catalogs:catalog-lakehouse-iceberg:copyLibAndConfig",
":catalogs:catalog-jdbc-mysql:copyCatalogLibs")
Copy link
Contributor

@jerryshao jerryshao Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you have a config file for jdbc-mysql catalog like other catalogs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

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 we should have to specify catalog level configurations like url, user, password, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought was that the catalog would expire, and users would need to pass in these attributes through a request. Now, I will add the conf configuration

...dbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/utils/JdbcConnectorUtils.java Show resolved Hide resolved
...sql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java
Comment on lines +102 to +107
String[] columnSpecs = columnDefinition.getColumnSpecs().toArray(new String[0]);
String columnProperties = String.join(SPACE, columnSpecs);
boolean nullable = !columnProperties.contains(NOT_NULL);
String defaultValue = findPropertiesValue(columnSpecs, DEFAULT);
String comment = findPropertiesValue(columnSpecs, COMMENT);
List<String> properties = getColumnProperties(columnProperties);
Optional.ofNullable(indexGroupByName.get(columnName)).ifPresent(properties::addAll);
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't fix this issue.

@Clearvive
Copy link
Contributor Author

@jerryshao The code for the properties of the column will be retained in this PR because we have not exposed the interface for users to use. The properties of the table will be ignored in this PR

@jerryshao jerryshao merged commit 4ab859f into apache:main Nov 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi1129 yuqi1129 yuqi1129 left review comments

@qqqttt123 qqqttt123 qqqttt123 left review comments

@jerryshao jerryshao jerryshao approved these changes

@mchades mchades Awaiting requested review from mchades

@FANNG1 FANNG1 Awaiting requested review from FANNG1

Assignees

@Clearvive Clearvive

Labels
None yet
Projects
None yet
Milestone
Gravitino 0.3.0
Development

Successfully merging this pull request may close these issues.

[Subtask] Support for mysql operations in JDBC catalog.
5 participants
@Clearvive @jerryshao @FANNG1 @yuqi1129 @qqqttt123