DanielCarter-stack commented on PR #10626:
URL: https://github.com/apache/seatunnel/pull/10626#issuecomment-4095409137
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10626", "part": 1,
"total": 1} -->
### Issue 1: Redundant setObject calls in AbstractJdbcRowConverter
**Location**: `AbstractJdbcRowConverter.java:219-221`
```java
if (fieldValue == null) {
setNullToStatementByDataType(
statement, seaTunnelDataType, statementIndex, sourceType);
statement.setObject(statementIndex, null); // ← Redundant call!
continue;
}
```
**Related Context**:
- Parent class: `AbstractJdbcRowConverter.java:240-247`
(`setNullToStatementByDataType` method)
- Subclass: `SqlserverJdbcRowConverter.java:58-71` (override implementation)
- Callers: `SimpleBatchStatementExecutor.java:48`,
`InsertOrUpdateBatchStatementExecutor.java:89,96,155`
**Problem Description**:
In the null branch of the `toExternal()` method,
`setNullToStatementByDataType()` is called first, followed by
`statement.setObject(statementIndex, null)`. This leads to:
1. **SQLServer Scenario**: `setNull(index, Types.LONGVARBINARY)` is called
first, then immediately followed by `setObject(index, null)` which overrides
the previous setting, **fix becomes ineffective**
2. **Other Database Scenarios**: The base class's
`setNullToStatementByDataType()` implementation also calls `setObject(index,
null)`, resulting in **setting the same parameter twice**
**Potential Risks**:
- **Risk 1 (High Severity)**: SQLServer fix is completely ineffective
because `setObject()` will override the type information from `setNull()`
- **Risk 2 (Medium Severity)**: All databases will execute redundant JDBC
calls, which doesn't affect functionality but impacts performance
- **Risk 3 (Low Severity)**: May cause confusion for certain JDBC drivers
(continuously setting the same parameter)
**Scope of Impact**:
- **Direct Impact**:
- `AbstractJdbcRowConverter.toExternal()` - Common logic for all JDBC
Converters
- `SqlserverJdbcRowConverter.setNullToStatementByDataType()` -
SQLServer-specific implementation (fix ineffective)
- **Indirect Impact**:
- JDBC Connectors for all 22 databases (MySQL, PostgreSQL, Oracle, etc.)
- All Sink scenarios using `SimpleBatchStatementExecutor` and
`InsertOrUpdateBatchStatementExecutor`
- **Impact Area**: Multiple Connectors / Core Framework
**Severity**: **CRITICAL** (Blocking issue, must be fixed before merging)
**Improvement Suggestions**:
```java
// AbstractJdbcRowConverter.java:218-223
if (fieldValue == null) {
setNullToStatementByDataType(
statement, seaTunnelDataType, statementIndex, sourceType);
// Delete this line: statement.setObject(statementIndex, null);
continue;
}
```
**Rationale**:
- `setNullToStatementByDataType()` is already responsible for setting null
values, including type information
- Redundant `setObject()` calls will override the correctly set type
information
- This is a core logic error in the PR that will cause the fix to be
completely ineffective
---
### Issue 2: Old toExternal method in SqlserverJdbcRowConverter not deleted
**Location**: `SqlserverJdbcRowConverter.java:91-164`
```java
public PreparedStatement toExternal(
SeaTunnelRowType rowType, SeaTunnelRow row, PreparedStatement
statement)
throws SQLException {
// ... Large amount of logic duplicated with parent class
}
```
**Related Context**:
- Parent class: `AbstractJdbcRowConverter.java:194-238` (new version of
`toExternal`)
- Interface: `JdbcRowConverter.java:43-56` (defines two overloaded versions)
- Caller: `SimpleBatchStatementExecutor.java:48` - calls the 4-parameter
version
**Problem Description**:
`SqlserverJdbcRowConverter` retains an old version of the `toExternal`
method (signature: `SeaTunnelRowType, SeaTunnelRow, PreparedStatement`). This
method:
1. Duplicates implementation logic from the parent class (approximately 70
lines of code)
2. **Will not be called** because all callers use the 4-parameter version
(with `databaseTableSchema`)
3. Contains **obsolete null handling logic** (Lines 98-101), which is
exactly the issue this PR aims to fix
4. Creates **confusion** with the parent class method, making future
maintainers unsure which one to modify
**Potential Risks**:
- **Risk 1 (Low Severity)**: Code redundancy increases maintenance costs
- **Risk 2 (Medium Severity)**: If future callers mistakenly use this
method, they will bypass the new null handling logic
- **Risk 3 (Medium Severity)**: The old null handling logic in this method
(Line 98: `if (fieldValue == null && seaTunnelDataType.getSqlType() !=
SqlType.BYTES)`) contradicts the fix claimed by the PR, indicating this method
is deprecated
**Scope of Impact**:
- **Direct Impact**: `SqlserverJdbcRowConverter` class itself
- **Indirect Impact**: May cause code review confusion and future errors
- **Impact Area**: Single Connector (SQLServer)
**Severity**: **MAJOR** (Does not block merging, but should be cleaned up)
**Improvement Suggestions**:
```java
// 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**:
- This method is legacy code from before PR #8031 and has been replaced by
the new 4-parameter version
- Keeping this method will mislead maintainers
- Deleting it can reduce approximately 70 lines of duplicate code
- If it确实 needs to be kept (such as for backward compatibility),
`@Deprecated` comments and clear documentation should be added
---
### Issue 3: Other databases may have the same issue
**Location**: All other JDBC Connector subclasses
**Related Context**:
- Other subclasses: `OracleJdbcRowConverter`, `DB2JdbcRowConverter`,
`PostgresJdbcRowConverter`, etc. (21 in total)
- Similar scenarios: Oracle's `BLOB`, `CLOB` types; DB2's `BLOB` type;
MySQL's `BLOB`, `BINARY` types
**Problem Description**:
This PR fixes the binary type null binding issue for SQLServer, but:
1. **Oracle**, **DB2**, **MySQL** and other databases also have similar
binary types (`BLOB`, `LONG RAW`, etc.)
2. These databases' JDBC drivers may have the same type inference issue
3. Currently, all these subclasses use the base class's default
implementation, which is `setObject(index, null)`
4. If they have the same problem, users will report similar bugs
**Potential Risks**:
- **Risk 1 (Medium Severity)**: Future users may report the same issue for
other databases
- **Risk 2 (Low Severity)**: Requires repetitive fix work (no unified
handling)
- **Risk 3 (Low Severity)**: Fixes for different databases may be
inconsistent
**Scope of Impact**:
- **Direct Impact**: None (currently only affects SQLServer)
- **Indirect Impact**: May need to apply similar fixes in other database
subclasses
- **Impact Area**: Multiple Connectors (potential)
**Severity**: **MINOR** (Does not block current PR, but worth considering)
**Improvement Suggestions**:
```java
// 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 Types
```
**Rationale**:
- This PR provides a good pattern that can be reused in other databases
- Suggestion: Merge the current PR first, then create a follow-up issue to
systematically check other databases
- Can search GitHub Issues to see if there are similar bug reports for other
databases
---
### Issue 4: Handling of unknown sourceType in resolveSqlServerBytesNullType
**Location**: `SqlserverJdbcRowConverter.java:73-89`
```java
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**:
- Type definitions: `SqlServerTypeConverter.java:76-90` (defines
`SQLSERVER_IMAGE`, `SQLSERVER_VARBINARY`, `SQLSERVER_BINARY`)
- Possible unknown types: User-defined types, legacy SQLServer types (e.g.,
`NTEXT`), future new types
**Problem Description**:
1. **When `sourceType == null`** (Lines 74-76), it defaults to returning
`VARBINARY`, which may be reasonable, but:
- If `databaseTableSchema == null`, we cannot obtain the actual source
type
- Using `VARBINARY` as a default value may not be applicable to certain
scenarios
2. **When `sourceType` is an unknown value** (Line 88), it also returns
`VARBINARY`:
- No log warning, users are unaware of the degradation
- If it's a future new SQLServer type, it may lead to silent errors
- If it's a configuration error, users cannot perceive it
**Potential Risks**:
- **Risk 1 (Medium Severity)**: Unknown types may need `LONGVARBINARY`
rather than `VARBINARY`
- **Risk 2 (Low Severity)**: Silent degradation, difficult to debug
- **Risk 3 (Low Severity)**: Future new SQLServer types may not be handled
correctly
**Scope of Impact**:
- **Direct Impact**:
`SqlserverJdbcRowConverter.resolveSqlServerBytesNullType()`
- **Indirect Impact**: Writing BYTES type null values to SQLServer
- **Impact Area**: Single Connector (SQLServer)
**Severity**: **MAJOR** (Recommended to improve before merging)
**Improvement Suggestions**:
```java
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**:
- For `sourceType == null`, using `VARBINARY` is reasonable (applicable to
most scenarios)
- For unknown types, using `LONGVARBINARY` is more conservative (can
accommodate larger data)
- Adding logs can help users diagnose issues
- Follows the "Robustness Principle" (Be Liberal in What You Accept)
**Alternatively**, if you don't want to add logs in production:
```java
// 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 comments
**Location**:
- `AbstractJdbcRowConverter.java:240-247` (`setNullToStatementByDataType`)
- `SqlserverJdbcRowConverter.java:58-71` (override implementation)
- `SqlserverJdbcRowConverter.java:73-89` (`resolveSqlServerBytesNullType`)
**Related Context**:
- Similar methods: `AbstractJdbcRowConverter.java:249-336`
(`setValueToStatementByDataType` - has complete switch-case comments)
**Problem Description**:
The two newly added methods have no JavaDoc or comments:
1. **`setNullToStatementByDataType()`** - Does not explain:
- Why this method is needed (instead of directly using `setObject(index,
null)`)
- Why subclasses need to override
- The purpose of parameter `sourceType`
2. **`resolveSqlServerBytesNullType()`** - Does not explain:
- Why SQLServer needs special handling
- Why each type maps to the corresponding JDBC Type
- The reason for `IMAGE` → `LONGVARBINARY`
**Potential Risks**:
- **Risk 1 (Low Severity)**: Future maintainers may not understand why this
method is needed
- **Risk 2 (Low Severity)**: May be mistakenly considered redundant code and
deleted
- **Risk 3 (Medium Severity)**: Other database developers won't know how to
apply the same pattern
**Scope of Impact**:
- **Direct Impact**: Code maintainability
- **Indirect Impact**: Knowledge transfer, future extensions
- **Impact Area**: Single Connector + potential impact on other Connectors
**Severity**: **MINOR** (Does not block merging, but strongly recommended to
add)
**Improvement Suggestions**:
```java
// 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**:
- Follows Apache project documentation standards
- Helps future maintainers understand design intent
- Provides reference for other database developers
- Aligns with "code as documentation" best practices
---
### Issue 6: Incomplete unit test coverage
**Location**: `SqlserverJdbcRowConverterTest.java`
**Related Context**:
- Test methods: `testSetNullToStatementByDataTypeForImage`,
`testSetNullToStatementByDataTypeForVarbinary`,
`testSetNullToStatementByDataTypeForBinary`,
`testSetNullToStatementByDataTypeForMaxVarbinary`
- Missing tests: `sourceType == null`, non-BYTES types, exception scenarios
**Problem Description**:
Current tests only cover 4 known sourceTypes, but are missing the following
scenarios:
1. **`sourceType == null`** scenario:
- Code handles this case at Lines 74-76
- But no test validation
- This is an actually possible scenario (when `databaseTableSchema ==
null`)
2. **Null values for non-BYTES types**:
- Code calls `setObject(index, null)` at Line 70
- But no test validation for null handling of non-BYTES types (such as
STRING, INT)
- Ensure no regression issues are introduced
3. **Unknown sourceType**:
- If an unrecognized type is passed (e.g., "UNKNOWN_BINARY")
- Should return `VARBINARY` (or improved `LONGVARBINARY`)
- But no test validates this fallback logic
4. **Compatibility with other databases**:
- No test validates that other databases (such as MySQL) are not affected
- Although this is a unit test, integration tests can be added
**Potential Risks**:
- **Risk 1 (Medium Severity)**: `sourceType == null` path is untested, may
have boundary issues
- **Risk 2 (Low Severity)**: Null handling for non-BYTES types has no
regression tests
- **Risk 3 (Low Severity)**: Fallback logic for unknown types is unverified
**Scope of Impact**:
- **Direct Impact**: Test coverage
- **Indirect Impact**: Future refactoring may break these untested paths
- **Impact Area**: Single Connector (SQLServer)
**Severity**: **MAJOR** (Recommended to supplement key tests before merging)
**Improvement Suggestions**:
```java
// 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**:
- These are critical path tests that should be covered
- Test cost is not high, but can significantly improve code quality
- Meets Apache project testing standards
- Can prevent future regression issues
---
### Issue 7: Missing integration tests or E2E tests
**Location**: PR's test coverage
**Related Context**:
- Existing SQLServer E2E tests:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-sqlserver-e2e` (from PR
#10214)
- PR description mentions "Manually verified with SQLServer", but no
automated tests
**Problem Description**:
1. **Missing end-to-end tests**:
- No tests for real SQLServer database connections
- No tests for actual data write scenarios
- Cannot verify the effectiveness of the fix in a real environment
2. **Missing cross-database tests**:
- No tests to verify other databases are not affected
- No tests to verify `databaseTableSchema` parameter passing
**Potential Risks**:
- **Risk 1 (Medium Severity)**: Unit tests pass, but real environment may
have issues
- **Risk 2 (Medium Severity)**: Cannot capture environment-related issues
(such as driver version)
- **Risk 3 (Low Severity)**: Manual testing cannot continuously validate
**Scope of Impact**:
- **Direct Impact**: Test coverage and confidence
- **Indirect Impact**: Future refactoring may break functionality
- **Impact Area**: Single Connector (SQLServer)
**Severity**: **MAJOR** (Strongly recommended to add, but does not block
merging)
**Improvement Suggestions**:
```java
// 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**:
- E2E tests can catch issues that unit tests cannot discover
- Meets Apache project testing standards
- Can prevent future regressions
- Recommended to add in a follow-up PR (does not block current PR)
---
--
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]
