Skip to content

Conversation

@chl-wxp
Copy link
Contributor

@chl-wxp chl-wxp commented Nov 21, 2025

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@chl-wxp chl-wxp marked this pull request as draft November 21, 2025 10:20
@github-actions github-actions bot added the e2e label Nov 25, 2025
@chl-wxp chl-wxp marked this pull request as ready for review November 25, 2025 15:21
@chl-wxp chl-wxp marked this pull request as draft November 28, 2025 01:33
@chl-wxp chl-wxp marked this pull request as ready for review November 28, 2025 14:10
Comment on lines +152 to +158
try {
return connection.createStatement().executeQuery(queryMeta).getMetaData();
} catch (SQLException e) {
throw new SeaTunnelRuntimeException(
JdbcConnectorErrorCode.NO_SUPPORT_OPERATION_FAILED,
"get meta data fail:" + schemaTableName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
return connection.createStatement().executeQuery(queryMeta).getMetaData();
} catch (SQLException e) {
throw new SeaTunnelRuntimeException(
JdbcConnectorErrorCode.NO_SUPPORT_OPERATION_FAILED,
"get meta data fail:" + schemaTableName);
}
try (Statement statement = connection.createStatement();
ResultSet resultSet = statement.executeQuery(queryMeta)) {
return resultSet.getMetaData();
} catch (SQLException e) {
throw new SeaTunnelRuntimeException(
JdbcConnectorErrorCode.NO_SUPPORT_OPERATION_FAILED,
"get meta data fail: " + schemaTableName, e);
}

Comment on lines +128 to +132
try {
connection.rollback();
} catch (SQLException rollbackEx) {
log.error("Failed to rollback", rollbackEx);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
connection.rollback();
} catch (SQLException rollbackEx) {
log.error("Failed to rollback", rollbackEx);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There‘s no need to handle rollback, JdbcOutputFormat will handle it

bulkCopy.setBulkCopyOptions(options);
long start = System.currentTimeMillis();
bulkCopy.writeToServer(new MemoryBulkData(resultSetMetaData, buffer));
connection.commit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
connection.commit();

Copy link
Contributor

Choose a reason for hiding this comment

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

SeaTunnel self will handle commit

Comment on lines +75 to +92
case DATE:
rowData[i] =
field == null
? null
: java.sql.Date.valueOf((java.time.LocalDate) field);
break;
case TIME:
rowData[i] =
field == null
? null
: java.sql.Time.valueOf((java.time.LocalTime) field);
break;
case TIMESTAMP:
rowData[i] =
field == null
? null
: java.sql.Timestamp.valueOf((java.time.LocalDateTime) field);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other types need to handle? like Decimal?

bulkCopy.setDestinationTableName(schemaTableName);
// BulkCopy config
SQLServerBulkCopyOptions options = new SQLServerBulkCopyOptions();
options.setTableLock(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is enabling table-level locks necessary?

Comment on lines +144 to +149
final String[] split = schemaTableName.split("\\.");
if (split.length != 2) {
Map<String, String> params = new HashMap<>();
params.put("value", schemaTableName);
throw new SeaTunnelRuntimeException(CommonErrorCode.SEATUNNEL_CONFIG_ERROR, params);
}
Copy link
Member

Choose a reason for hiding this comment

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

Does it have to be schema.tableName? How about we allow users to only configure tableName, and in this case, the default schema will be used?

sink {
  Jdbc {
    // ...
    table = "my_target_table"  
  }
}

Comment on lines +27 to +29
import com.microsoft.sqlserver.jdbc.ISQLServerBulkData;
import com.microsoft.sqlserver.jdbc.SQLServerBulkCopy;
import com.microsoft.sqlserver.jdbc.SQLServerBulkCopyOptions;
Copy link
Member

Choose a reason for hiding this comment

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

@chl-wxp This strong dependency will lead to a strong reliance, where mssql-jdbc is imported regardless of whether it is used, which is inconsistent with the previous design.
cc @davidzollo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chl-wxp This strong dependency will lead to a strong reliance, where mssql-jdbc is imported regardless of whether it is used, which is inconsistent with the previous design. cc @davidzollo

Thanks for the reminder. Such strong dependency is indeed a problem. I need to think about how to solve this problem.

@chl-wxp chl-wxp marked this pull request as draft December 4, 2025 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants