[Bug][Connect-V2][Jdbc][Sqlserver] Fix JDBC sink null binding for SQLServer binary-like columns (e.g. IMAGE)#10626
Open
siwen-yu wants to merge 2 commits intoapache:devfrom
Open
[Bug][Connect-V2][Jdbc][Sqlserver] Fix JDBC sink null binding for SQLServer binary-like columns (e.g. IMAGE)#10626siwen-yu wants to merge 2 commits intoapache:devfrom
siwen-yu wants to merge 2 commits intoapache:devfrom
Conversation
siwen-yu
force-pushed
the
hotfix/fix-10613
branch
from
7e0591a to
0f8e985
Compare
…Server binary-like columns (e.g. IMAGE)
Issue 1: Redundant setObject calls in AbstractJdbcRowConverterLocation: if (fieldValue == null) {
setNullToStatementByDataType(
statement, seaTunnelDataType, statementIndex, sourceType);
statement.setObject(statementIndex, null); // ← Redundant call!
continue;
}Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: CRITICAL (Blocking issue, must be fixed before merging) Improvement Suggestions: // AbstractJdbcRowConverter.java:218-223
if (fieldValue == null) {
setNullToStatementByDataType(
statement, seaTunnelDataType, statementIndex, sourceType);
// Delete this line: statement.setObject(statementIndex, null);
continue;
}Rationale:
Issue 2: Old toExternal method in SqlserverJdbcRowConverter not deletedLocation: public PreparedStatement toExternal(
SeaTunnelRowType rowType, SeaTunnelRow row, PreparedStatement statement)
throws SQLException {
// ... Large amount of logic duplicated with parent class
}Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MAJOR (Does not block merging, but should be cleaned up) Improvement Suggestions: // Delete the entire method at SqlserverJdbcRowConverter.java:91-164
// Reason:
// 1. This method will not be called (all callers use the 4-parameter version)
// 2. Logic duplicated with parent class
// 3. Contains old, buggy null handling logic
// 4. Deletion does not affect any functionality (parent class method will take effect)Rationale:
Issue 3: Other databases may have the same issueLocation: All other JDBC Connector subclasses Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MINOR (Does not block current PR, but worth considering) Improvement Suggestions: // Solution 1 (Recommended): Add similar overrides in other database subclasses
// For example OracleJdbcRowConverter.java:
@Override
protected void setNullToStatementByDataType(
PreparedStatement statement,
SeaTunnelDataType<?> seaTunnelDataType,
int statementIndex,
@Nullable String sourceType)
throws SQLException {
if (seaTunnelDataType.getSqlType() == SqlType.BYTES) {
// Oracle BLOB type requires special handling
statement.setNull(statementIndex, java.sql.Types.BLOB);
return;
}
statement.setObject(statementIndex, null);
}
// Solution 2 (Long-term optimization): Provide generic mapping in AbstractJdbcRowConverter
// Use a Map<SqlType, Integer> to store default JDBC TypesRationale:
Issue 4: Handling of unknown sourceType in resolveSqlServerBytesNullTypeLocation: private int resolveSqlServerBytesNullType(@Nullable String sourceType) {
if (sourceType == null) {
return java.sql.Types.VARBINARY; // ← Is the default value reasonable?
}
if (SqlServerTypeConverter.SQLSERVER_IMAGE.equals(sourceType)) {
return java.sql.Types.LONGVARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_VARBINARY)) {
return java.sql.Types.VARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_BINARY)) {
return java.sql.Types.BINARY;
}
return java.sql.Types.VARBINARY; // ← Return VARBINARY for unknown types?
}Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MAJOR (Recommended to improve before merging) Improvement Suggestions: private int resolveSqlServerBytesNullType(@Nullable String sourceType) {
if (sourceType == null) {
log.warn(
"Cannot determine source type for BYTES field, defaulting to VARBINARY. " +
"If this causes errors, please provide databaseTableSchema.");
return java.sql.Types.VARBINARY;
}
if (SqlServerTypeConverter.SQLSERVER_IMAGE.equals(sourceType)) {
return java.sql.Types.LONGVARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_VARBINARY)) {
return java.sql.Types.VARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_BINARY)) {
return java.sql.Types.BINARY;
}
// Unknown type, log warning and use conservative LONGVARBINARY
log.warn(
"Unknown SQLServer binary type: {}, defaulting to LONGVARBINARY. " +
"This may cause issues if the target column is not a large binary type.",
sourceType);
return java.sql.Types.LONGVARBINARY; // More conservative default value
}Rationale:
Alternatively, if you don't want to add logs in production: // Minimize changes: only modify return value for unknown types
private int resolveSqlServerBytesNullType(@Nullable String sourceType) {
if (sourceType == null) {
return java.sql.Types.VARBINARY;
}
if (SqlServerTypeConverter.SQLSERVER_IMAGE.equals(sourceType)) {
return java.sql.Types.LONGVARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_VARBINARY)) {
return java.sql.Types.VARBINARY;
}
if (sourceType.startsWith(SqlServerTypeConverter.SQLSERVER_BINARY)) {
return java.sql.Types.BINARY;
}
// Use LONGVARBINARY for unknown types (more conservative)
return java.sql.Types.LONGVARBINARY;
}Issue 5: Missing JavaDoc and commentsLocation:
Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MINOR (Does not block merging, but strongly recommended to add) Improvement Suggestions: // AbstractJdbcRowConverter.java:240-247
/**
* Bind a null value to the PreparedStatement parameter.
*
* <p>Default implementation uses {@link PreparedStatement#setObject(int, Object)},
* which works for most databases. However, some databases (e.g., SQL Server) require
* explicit type information for null values in certain column types (e.g., IMAGE, BLOB).
*
* <p>Subclasses can override this method to provide database-specific null handling.
* For example, SQL Server uses {@link PreparedStatement#setNull(int, int)} with
* the appropriate JDBC type (e.g., {@link java.sql.Types#LONGVARBINARY} for IMAGE columns).
*
* @param statement the PreparedStatement to bind the parameter to
* @param seaTunnelDataType the SeaTunnel data type of the field
* @param statementIndex the 1-based parameter index in the PreparedStatement
* @param sourceType the source database column type (if available), used to determine
* the appropriate JDBC type for null values
* @throws SQLException if a database access error occurs
*/
protected void setNullToStatementByDataType(
PreparedStatement statement,
SeaTunnelDataType<?> seaTunnelDataType,
int statementIndex,
@Nullable String sourceType)
throws SQLException {
statement.setObject(statementIndex, null);
}
// SqlserverJdbcRowConverter.java:58-89
/**
* {@inheritDoc}
*
* <p>SQL Server requires explicit type information for null values in binary columns
* (IMAGE, VARBINARY, BINARY). Using {@code setObject(index, null)} causes the driver to
* infer the type as NVARCHAR, which leads to conversion errors:
* <pre>
* Implicit conversion from data type nvarchar to image is not allowed
* </pre>
*
* <p>This method uses {@code setNull(index, sqlType)} with the appropriate JDBC type
* based on the source column type.
*/
@Override
protected void setNullToStatementByDataType(
PreparedStatement statement,
SeaTunnelDataType<?> seaTunnelDataType,
int statementIndex,
@Nullable String sourceType)
throws SQLException {
if (seaTunnelDataType.getSqlType() == SqlType.BYTES) {
statement.setNull(statementIndex, resolveSqlServerBytesNullType(sourceType));
return;
}
statement.setObject(statementIndex, null);
}
/**
* Resolves the appropriate JDBC type for SQL Server binary columns when the value is null.
*
* <p>SQL Server has multiple binary types, each mapping to a different JDBC type:
* <ul>
* <li>IMAGE → LONGVARBINARY (legacy large binary type)</li>
* <li>VARBINARY(*) → VARBINARY</li>
* <li>VARBINARY(MAX) → VARBINARY</li>
* <li>BINARY(*) → BINARY (fixed-length)</li>
* </ul>
*
* @param sourceType the SQL Server column type (e.g., "IMAGE", "VARBINARY", "BINARY")
* @return the JDBC type constant from {@link java.sql.Types}
*/
private int resolveSqlServerBytesNullType(@Nullable String sourceType) {
// ... implementation
}Rationale:
Issue 6: Incomplete unit test coverageLocation: Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MAJOR (Recommended to supplement key tests before merging) Improvement Suggestions: // SqlserverJdbcRowConverterTest.java
@Test
void testSetNullToStatementByDataTypeForNullSourceType() throws Exception {
// Test when sourceType is null, use default VARBINARY
PreparedStatement statement = mock(PreparedStatement.class);
SqlserverJdbcRowConverter converter = new SqlserverJdbcRowConverter();
SeaTunnelDataType<?> bytesType = PrimitiveByteArrayType.INSTANCE;
converter.setNullToStatementByDataType(
statement, bytesType, 1, null);
verify(statement).setNull(1, Types.VARBINARY);
}
@Test
void testSetNullToStatementByDataTypeForNonBytesType() throws Exception {
// Test null values of non-BYTES type should use setObject
PreparedStatement statement = mock(PreparedStatement.class);
SqlserverJdbcRowConverter converter = new SqlserverJdbcRowConverter();
SeaTunnelDataType<?> stringType = BasicType.STRING_TYPE;
converter.setNullToStatementByDataType(
statement, stringType, 1, null);
verify(statement).setObject(1, null);
}
@Test
void testSetNullToStatementByDataTypeForUnknownSourceType() throws Exception {
// Test unknown sourceType should use default type
PreparedStatement statement = mock(PreparedStatement.class);
SqlserverJdbcRowConverter converter = new SqlserverJdbcRowConverter();
SeaTunnelDataType<?> bytesType = PrimitiveByteArrayType.INSTANCE;
converter.setNullToStatementByDataType(
statement, bytesType, 1, "UNKNOWN_BINARY_TYPE");
// According to improvement suggestions, should return LONGVARBINARY or VARBINARY
verify(statement).setNull(1, Types.LONGVARBINARY); // Or Types.VARBINARY
}Rationale:
Issue 7: Missing integration tests or E2E testsLocation: PR's test coverage Related Context:
Problem Description:
Potential Risks:
Scope of Impact:
Severity: MAJOR (Strongly recommended to add, but does not block merging) Improvement Suggestions: // Add tests in seatunnel-e2e/seatunnel-connector-v2-e2e/connector-sqlserver-e2e
@Test
public void testWriteNullToBinaryColumns() throws Exception {
// Create table with IMAGE, VARBINARY, BINARY columns
String createTableSql =
"CREATE TABLE test_binary_null (\n" +
" id INT,\n" +
" image_col IMAGE NULL,\n" +
" varbinary_col VARBINARY(100) NULL,\n" +
" binary_col BINARY(10) NULL\n" +
")";
// Insert data containing null values
// Verify data is successfully written
// Verify null values are correctly saved
}
@Test
public void testWriteMixedNullAndNonNull() throws Exception {
// Test scenarios containing both null and non-null values
}Rationale:
|
Author
|
@DanielCarter-stack Thanks for the detailed and thorough review — I’ve gone through all the points carefully. Changes to be addressed in this PRIssue 1 Issue 2 Issue 4 Issue 5 Issue 6
Follow-upsIssue 3 Issue 7 Thanks again for the valuable feedback — I’ll update the PR soon. |
…ary types and clean up legacy logic. - Fix null binding override issue in AbstractJdbcRowConverter - Remove legacy unused toExternal(...) method - Improve fallback handling for unknown sourceType with warning - Add JavaDoc for SQLServer null binding behavior - Extend unit test coverage
siwen-yu
force-pushed
the
hotfix/fix-10613
branch
from
67c8b07 to
5dccaad
Compare
Author
|
Addressed review comments in the latest update:
Please take another look. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this pull request
fix #10613
Previously, when a field value was null,
AbstractJdbcRowConverterused:statement.setObject(index, null)
On SQLServer, this may cause the driver to infer
nvarchartype for null values,which leads to errors like:
"Implicit conversion from data type nvarchar to image is not allowed"
Does this PR introduce any user-facing change?
No.
This is a bug fix that corrects null handling for SQLServer binary columns.
Existing jobs will continue to work as before, except that previously failing
cases (e.g. writing null to IMAGE column) will now succeed.
How was this patch tested?
Added unit tests in
SqlserverJdbcRowConverterTestVerified logic through code path`
Manually verified with SQLServer
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.