Hisoka-X commented on code in PR #7288:
URL: https://github.com/apache/seatunnel/pull/7288#discussion_r1701633356
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/sink/JdbcSink.java:
##########
@@ -194,7 +194,10 @@ public Optional<SaveModeHandler> getSaveModeHandler() {
return Optional.empty();
}
Optional<Catalog> catalogOptional =
-
JdbcCatalogUtils.findCatalog(jdbcSinkConfig.getJdbcConnectionConfig(), dialect);
+ JdbcCatalogUtils.findCatalog(
+ jdbcSinkConfig.getJdbcConnectionConfig(),
+ dialect,
+ jdbcSinkConfig.isCreateIndex());
Review Comment:
Why catalog should care aboult `createIndex` value at now?
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/iris/IrisCreateTableSqlBuilder.java:
##########
@@ -40,14 +40,16 @@ public class IrisCreateTableSqlBuilder {
private String fieldIde;
private String comment;
+ private Boolean createIndex;
Review Comment:
```suggestion
private boolean createIndex;
```
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oracle/OracleCreateTableSqlBuilder.java:
##########
@@ -38,12 +38,14 @@ public class OracleCreateTableSqlBuilder {
private PrimaryKey primaryKey;
private String sourceCatalogName;
private String fieldIde;
+ private Boolean createIndex;
- public OracleCreateTableSqlBuilder(CatalogTable catalogTable) {
+ public OracleCreateTableSqlBuilder(CatalogTable catalogTable, Boolean
createIndex) {
Review Comment:
ditto
##########
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-3/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcPostgresIT.java:
##########
@@ -297,21 +297,31 @@ public void testCatalog() {
TablePath tablePath = new TablePath(databaseName, schema, tableName);
TablePath catalogTablePath = new TablePath(catalogDatabaseName,
schema, catalogTableName);
-
Assertions.assertFalse(catalog.databaseExists(catalogTablePath.getDatabaseName()));
- catalog.createDatabase(catalogTablePath, false);
-
Assertions.assertTrue(catalog.databaseExists(catalogTablePath.getDatabaseName()));
+ Lists.newArrayList(true, false)
Review Comment:
ditto
##########
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-common/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/AbstractJdbcIT.java:
##########
@@ -350,37 +351,48 @@ protected void initCatalog() {}
@Test
public void testCatalog() {
- if (catalog == null) {
- return;
- }
-
- TablePath sourceTablePath =
- new TablePath(
- jdbcCase.getDatabase(), jdbcCase.getSchema(),
jdbcCase.getSourceTable());
- TablePath targetTablePath =
- new TablePath(
- jdbcCase.getCatalogDatabase(),
- jdbcCase.getCatalogSchema(),
- jdbcCase.getCatalogTable());
- boolean createdDb = false;
-
- if (!catalog.databaseExists(targetTablePath.getDatabaseName())) {
- catalog.createDatabase(targetTablePath, false);
-
Assertions.assertTrue(catalog.databaseExists(targetTablePath.getDatabaseName()));
- createdDb = true;
- }
-
- CatalogTable catalogTable = catalog.getTable(sourceTablePath);
- catalog.createTable(targetTablePath, catalogTable, false);
- Assertions.assertTrue(catalog.tableExists(targetTablePath));
-
- catalog.dropTable(targetTablePath, false);
- Assertions.assertFalse(catalog.tableExists(targetTablePath));
-
- if (createdDb) {
- catalog.dropDatabase(targetTablePath, false);
-
Assertions.assertFalse(catalog.databaseExists(targetTablePath.getDatabaseName()));
- }
+ Lists.newArrayList(true, false)
Review Comment:
It is recommended to test create index or not in single test case. This
method of mixing it with existing test cases is difficult to maintain.
Also, I do not find any assert logic to make sure the index not created.
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/iris/IrisCreateTableSqlBuilder.java:
##########
@@ -40,14 +40,16 @@ public class IrisCreateTableSqlBuilder {
private String fieldIde;
private String comment;
+ private Boolean createIndex;
- public IrisCreateTableSqlBuilder(CatalogTable catalogTable) {
+ public IrisCreateTableSqlBuilder(CatalogTable catalogTable, Boolean
createIndex) {
Review Comment:
```suggestion
public IrisCreateTableSqlBuilder(CatalogTable catalogTable, boolean
createIndex) {
```
##########
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-2/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcOceanBaseOracleIT.java:
##########
@@ -220,20 +220,29 @@ protected void initCatalog() {
@Test
@Override
public void testCatalog() {
- TablePath sourceTablePath =
- new TablePath(
- jdbcCase.getDatabase(), jdbcCase.getSchema(),
jdbcCase.getSourceTable());
- TablePath targetTablePath =
- new TablePath(
- jdbcCase.getCatalogDatabase(),
- jdbcCase.getCatalogSchema(),
- jdbcCase.getCatalogTable());
-
- CatalogTable catalogTable = catalog.getTable(sourceTablePath);
- catalog.createTable(targetTablePath, catalogTable, false);
- Assertions.assertTrue(catalog.tableExists(targetTablePath));
-
- catalog.dropTable(targetTablePath, false);
- Assertions.assertFalse(catalog.tableExists(targetTablePath));
+ Lists.newArrayList(true, false)
Review Comment:
ditto
##########
seatunnel-api/src/main/java/org/apache/seatunnel/api/table/catalog/Catalog.java:
##########
@@ -239,6 +239,23 @@ default <T> void buildColumnsWithErrorCheck(
void createTable(TablePath tablePath, CatalogTable table, boolean
ignoreIfExists)
throws TableAlreadyExistException, DatabaseNotExistException,
CatalogException;
+ /**
+ * Create a new table in this catalog.
+ *
+ * @param tablePath Path of the table
+ * @param table The table definition
+ * @param ignoreIfExists Flag to specify behavior when a table with the
given name already exist
+ * @param createIndex If you want to create index or not
+ * @throws TableAlreadyExistException thrown if the table already exists
in the catalog and
+ * ignoreIfExists is false
+ * @throws DatabaseNotExistException thrown if the database in tablePath
doesn't exist in the
+ * catalog
+ * @throws CatalogException in case of any runtime exception
+ */
+ default void createTable(
+ TablePath tablePath, CatalogTable table, boolean ignoreIfExists,
boolean createIndex)
+ throws TableAlreadyExistException, DatabaseNotExistException,
CatalogException {}
Review Comment:
```suggestion
throws TableAlreadyExistException, DatabaseNotExistException,
CatalogException {
createTable(tablePaht, table, ignoreIfExists)
}
```
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/sink/JdbcSink.java:
##########
@@ -219,16 +222,18 @@ public Optional<SaveModeHandler> getSaveModeHandler() {
catalog,
tablePath,
catalogTable,
- config.get(JdbcOptions.CUSTOM_SQL)));
+ config.get(JdbcOptions.CUSTOM_SQL),
+ jdbcSinkConfig.isCreateIndex()));
Review Comment:
ditto
##########
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-3/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcSqlServerIT.java:
##########
@@ -341,34 +341,49 @@ public void testCatalog() {
sqlServerCatalog.executeSql(
tablePathSqlserver,
"execute sp_addextendedproperty
'MS_Description','\"#¥%……&*();\\\\;'',,..``````//''@Xx''\\''\"','user','dbo','table','source','column','BIGINT_TEST';");
- CatalogTable catalogTable =
sqlServerCatalog.getTable(tablePathSqlserver);
- // sink tableExists ?
- boolean tableExistsBefore =
sqlServerCatalog.tableExists(tablePathSqlserverSink);
- Assertions.assertFalse(tableExistsBefore);
- // create table
- sqlServerCatalog.createTable(tablePathSqlserverSink, catalogTable,
true);
- boolean tableExistsAfter =
sqlServerCatalog.tableExists(tablePathSqlserverSink);
- Assertions.assertTrue(tableExistsAfter);
- // comment
- final CatalogTable sinkTable =
sqlServerCatalog.getTable(tablePathSqlserverSink);
- Assertions.assertEquals(
- sinkTable.getTableSchema().getColumns().get(1).getComment(),
- "\"#¥%……&*();\\\\;',,..``````//'@Xx'\\'\"");
- // isExistsData ?
- boolean existsDataBefore =
sqlServerCatalog.isExistsData(tablePathSqlserverSink);
- Assertions.assertFalse(existsDataBefore);
- // insert one data
- sqlServerCatalog.executeSql(
- tablePathSqlserverSink,
- "insert into sink_lw(INT_IDENTITY_TEST, BIGINT_TEST) values(1,
12)");
- boolean existsDataAfter =
sqlServerCatalog.isExistsData(tablePathSqlserverSink);
- Assertions.assertTrue(existsDataAfter);
- // truncateTable
- sqlServerCatalog.truncateTable(tablePathSqlserverSink, true);
-
Assertions.assertFalse(sqlServerCatalog.isExistsData(tablePathSqlserverSink));
- // drop table
- sqlServerCatalog.dropTable(tablePathSqlserverSink, true);
-
Assertions.assertFalse(sqlServerCatalog.tableExists(tablePathSqlserverSink));
- sqlServerCatalog.close();
+ catalog.close();
+ Lists.newArrayList(true, false)
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
