DanielCarter-stack commented on PR #10453:
URL: https://github.com/apache/seatunnel/pull/10453#issuecomment-3851791141
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10453", "part": 1,
"total": 1} -->
### Issue 1: Missing Unit Tests
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverterTest.java`
**Related Context**:
- Test class: `MySqlTypeConverterTest.java`
- Existing test methods: `testConvertSet()` (lines 1092-1105)
**Issue Description**:
The PR modifies the `MySqlTypeConverter.convert()` method, adding support
for the `MYSQL_SET_UNSIGNED` type, but does not add corresponding unit tests.
Although the existing code already has `testConvertSet()` tests for the
`MYSQL_SET` type, `MYSQL_SET_UNSIGNED` is a different type string and requires
independent test case verification.
**Potential Risks**:
- Risk 1: If the format returned by MySQL Connector/J does not match
expectations, tests cannot detect this
- Risk 2: During code review, the correctness of type conversion logic
cannot be verified
- Risk 3: Future refactoring may break this functionality without detection
**Impact Scope**:
- Direct impact: `MySqlTypeConverter.convert()` method
- Indirect impact: Users using MySQL CDC/JDBC Connector with `SET UNSIGNED`
types in their tables
**Severity**: MAJOR
**Improvement Suggestions**:
```java
@Test
public void testConvertSetUnsigned() {
// Test normal SET UNSIGNED type
BasicTypeDefine<Object> typeDefine =
BasicTypeDefine.builder()
.name("test")
.columnType("SET('reading','sports','music','travel')
UNSIGNED")
.dataType("SET UNSIGNED")
.length(3L)
.build();
Column column = MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefine);
Assertions.assertEquals(typeDefine.getName(), column.getName());
Assertions.assertEquals(BasicType.STRING_TYPE, column.getDataType());
Assertions.assertEquals(3, column.getColumnLength());
Assertions.assertEquals(typeDefine.getColumnType(),
column.getSourceType());
// Test SET UNSIGNED type with empty length
BasicTypeDefine<Object> typeDefineNoLength =
BasicTypeDefine.builder()
.name("test_no_length")
.columnType("SET UNSIGNED")
.dataType("SET UNSIGNED")
.build();
Column columnNoLength =
MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefineNoLength);
Assertions.assertEquals(BasicType.STRING_TYPE,
columnNoLength.getDataType());
Assertions.assertEquals(100, columnNoLength.getColumnLength()); //
Default length
}
```
**Rationale**:
1. Unit tests are a basic requirement for PRs (the PR description checklist
mentions "make sure that your code changes are covered with tests")
2. Tests should cover both normal and edge cases (when length is null)
3. Reference the `testConvertSet()` pattern to maintain consistent test style
---
### Issue 2: Missing E2E Test Coverage
**Location**:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-cdc-mysql-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/AbstractMysqlCDCITBase.java`
**Related Context**:
- Test base class: `AbstractMysqlCDCITBase.java`
- Table creation statement: The INSERT statement around line 706 contains
`f_enum` column but no `f_set` column
**Issue Description**:
The PR description mentions fixing the `SET UNSIGNED` type support issue in
MySQL CDC scenarios, but the existing E2E tests **do not include columns of SET
or SET UNSIGNED types**. This means:
1. The actual effect of the fix cannot be verified in the E2E environment
2. Problems users encounter in production may not be detected in CI/CD
Reviewing the table creation and test data insertion logic in
`AbstractMysqlCDCITBase`:
- The query template includes `f_enum` column (lines 77, 86)
- INSERT statements insert `f_enum` value as `'enum2'` (lines 715, 741)
- But **no** `f_set` or `f_set_unsigned` column
**Potential Risks**:
- Risk 1: The modification may not work in certain edge cases (e.g.,
specific MySQL versions, Connector/J versions)
- Risk 2: Cannot verify that the end-to-end data flow is correct
- Risk 3: Regression testing cannot be guaranteed
**Impact Scope**:
- Direct impact: E2E test coverage rate of MySQL CDC Connector
- Indirect impact: Users using SET UNSIGNED type encountering problems in
production environments
**Severity**: MAJOR
**Improvement Suggestions**:
Add SET UNSIGNED type test columns in `AbstractMysqlCDCITBase`:
1. Modify the query template (around lines 72-78):
```java
private static final String SOURCE_SQL_TEMPLATE =
"select id, cast(f_binary as char) as f_binary, ..., f_enum, f_set,
f_set_unsigned, ... "
// Add f_set and f_set_unsigned after f_enum
```
2. Add SET column in the table creation statement (need to find the SQL for
table creation):
```sql
CREATE TABLE ... (
...
f_enum ENUM('enum1', 'enum2', 'enum3'),
f_set SET('reading', 'sports', 'music', 'travel'),
f_set_unsigned SET('a', 'b', 'c') UNSIGNED,
...
)
```
3. Add values for SET column in INSERT statements:
```java
+ " f_timestamp, f_bit1, f_bit64,
f_char, f_enum, f_set, f_set_unsigned, f_mediumblob, ...\n"
// Add in VALUES:
+ " '2023-04-27 11:08:40', 1, b'010101...', 'C', 'enum2',
'reading,travel', 'a,b', ...\n"
```
**Rationale**:
1. E2E testing is critical for verifying CDC functionality correctness
2. SET types have special data formats in MySQL (comma-separated strings)
3. UNSIGNED attribute may cause different type names to be returned,
requiring actual testing for verification
---
### Issue 3: reconvert Method Does Not Support SET Type Reverse Conversion
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:334-550`
**Related Context**:
- `convert()` method: lines 134-331, converts MySQL types to SeaTunnel types
- `reconvert()` method: lines 334-550, converts SeaTunnel types back to
MySQL types
- STRING type reconvert: lines 465-487
**Issue Description**:
The `convert()` method converts both `MYSQL_SET` and `MYSQL_SET_UNSIGNED` to
`BasicType.STRING_TYPE`, which is reasonable. However, the `reconvert()`
method, when handling `BasicType.STRING_TYPE` (lines 465-487):
```java
case STRING:
if (column.getColumnLength() == null || column.getColumnLength() <= 0) {
builder.nativeType(MysqlType.LONGTEXT);
builder.columnType(MYSQL_LONGTEXT);
builder.dataType(MYSQL_LONGTEXT);
} else if (column.getColumnLength() < POWER_2_8) {
builder.nativeType(MysqlType.VARCHAR);
builder.columnType(String.format("%s(%s)", MYSQL_VARCHAR,
column.getColumnLength()));
builder.dataType(MYSQL_VARCHAR);
} else if (column.getColumnLength() < POWER_2_16) {
builder.nativeType(MysqlType.TEXT);
builder.columnType(MYSQL_TEXT);
builder.dataType(MYSQL_TEXT);
} else if (column.getColumnLength() < POWER_2_24) {
builder.nativeType(MysqlType.MEDIUMTEXT);
builder.columnType(MYSQL_MEDIUMTEXT);
builder.dataType(MYSQL_MEDIUMTEXT);
} else {
builder.nativeType(MysqlType.LONGTEXT);
builder.columnType(MYSQL_LONGTEXT);
builder.dataType(MYSQL_LONGTEXT);
}
break;
```
This leads to **type information loss**:
- Read SET column from MySQL → SeaTunnel STRING
- Write SeaTunnel STRING back to MySQL → VARCHAR/TEXT/LONGTEXT (instead of
SET)
**Potential Risks**:
- Risk 1: When users use SeaTunnel for database migration, SET type will be
converted to VARCHAR/TEXT
- Risk 2: If downstream systems depend on SET type constraints (e.g., can
only insert predefined values), these constraints are lost
- Risk 3: In schema synchronization scenarios, table structure changes
unexpectedly
**Impact Scope**:
- Direct impact: Users using MySQL JDBC Sink
- Indirect impact: Database migration, schema synchronization scenarios
**Severity**: MINOR (because CDC scenarios are typically one-way sync, not
involving write-back)
**Improvement Suggestions**:
This is a **design limitation**; a complete fix requires:
**Solution 1: Preserve original type information in Column**
```java
// Add field in PhysicalColumn
private String originalSourceType; // Keep original MySQL type name
// Check in reconvert
case STRING:
if ("SET".equals(column.getOriginalSourceType())
|| "SET UNSIGNED".equals(column.getOriginalSourceType())) {
builder.nativeType(MysqlType.SET);
builder.columnType(column.getSourceType()); // Use original type
builder.dataType("SET");
} else {
// Existing logic
}
```
**Solution 2: Add Column metadata extension**
```java
// Use SeaTunnel's Column metadata mechanism
column.getMetadata().put("mysql.type.variant", "UNSIGNED");
```
**Rationale**:
1. This is a known design limitation, not only affecting SET types
2. ENUM types have the same problem
3. A complete fix requires API-level changes, which is beyond the scope of
this PR
4. It is reasonable to document this limitation
**Suggested Documentation Update**:
Add a note in `docs/en/connectors/connector-jdbc.md` or related
documentation:
> **Type Mapping Note**: When reading MySQL SET or ENUM types, they are
mapped to SeaTunnel STRING type. When writing back to MySQL, STRING types are
mapped to VARCHAR/TEXT based on length. The original SET/ENUM constraints are
not preserved. If you need to preserve the exact table structure, use DDL
synchronization tools instead.
---
### Issue 4: Missing Comments and Documentation
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:79`
**Related Context**:
- Constant definitions: lines 44-96
- MYSQL_SET constant: line 78
**Issue Description**:
The PR adds the `MYSQL_SET_UNSIGNED` constant but does not add any
explanatory comments for:
1. Why this constant is needed
2. Under what circumstances MySQL returns "SET UNSIGNED" as a type name
3. What the specific content of Issue #10451 is
4. How this differs from regular SET types
Reviewing the definitions of other constants:
```java
static final String MYSQL_SET = "SET"; // No comments
static final String MYSQL_ENUM = "ENUM"; // No comments
```
In the existing code, most type constants lack comments, which is an
**overall code quality issue**. However, for this **rare and easily confusing**
type, comments should be added.
**Potential Risks**:
- Risk 1: Future maintainers may not understand why this constant is needed
and mistakenly think it can be deleted
- Risk 2: Code reviewers cannot judge the correctness of the modification
- Risk 3: Users cannot find clues from the code when encountering problems
**Impact Scope**:
- Direct impact: Code maintainability
- Indirect impact: Problem troubleshooting efficiency
**Severity**: MINOR
**Improvement Suggestions**:
```java
/**
* MySQL SET type with UNSIGNED attribute.
*
* <p>In certain MySQL versions or configurations, the JDBC driver may return
* "SET UNSIGNED" as the type name for SET columns with the UNSIGNED
attribute.
* This is different from the regular SET type and needs special handling.
*
* <p>See Issue #10451 for more details.
*
* <p>Example DDL that may trigger this:
* <pre>
* CREATE TABLE test (
* status SET('ready','processing','done') UNSIGNED
* );
* </pre>
*/
static final String MYSQL_SET_UNSIGNED = "SET UNSIGNED";
```
Comments can also be added near the switch case:
```java
// Handle SET types (with or without UNSIGNED attribute)
// MySQL Connector/J may return "SET" or "SET UNSIGNED" depending on the
column definition
case MYSQL_ENUM:
case MYSQL_SET:
case MYSQL_SET_UNSIGNED:
builder.dataType(BasicType.STRING_TYPE);
...
```
**Rationale**:
1. Comments should explain "why" rather than just "what"
2. Providing background information helps future maintainers understand the
context
3. Referencing the Issue link facilitates tracing design decisions
---
### Issue 5: Possibility of ENUM UNSIGNED Not Considered
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:248-257`
**Related Context**:
- ENUM and SET case branches: lines 248-257
- Both use the same conversion logic: `BasicType.STRING_TYPE`
**Issue Description**:
The PR only adds support for `MYSQL_SET_UNSIGNED` but does not consider the
possibility of `ENUM UNSIGNED`.
From MySQL's type system perspective:
- SET and ENUM are both special string types
- SET allows storing combinations of multiple values
- ENUM only allows storing a single predefined value
- Both can have UNSIGNED attribute added (though extremely rare)
If MySQL supports `ENUM UNSIGNED`, and MySQL Connector/J returns `"ENUM
UNSIGNED"` in some cases, the same problem will recur.
**Verification**:
Need to consult MySQL official documentation and MySQL Connector/J source
code to confirm:
1. Whether MySQL supports `ENUM UNSIGNED` syntax
2. Whether MySQL Connector/J returns `"ENUM UNSIGNED"` as a type name
**Potential Risks**:
- Risk 1: If `ENUM UNSIGNED` exists, users will encounter the same problem
as Issue #10451
- Risk 2: Another PR will be needed to fix ENUM UNSIGNED
**Impact Scope**:
- Direct impact: Users who might use ENUM UNSIGNED
- Indirect impact: Code consistency and completeness
**Severity**: MINOR (because ENUM UNSIGNED is extremely rare)
**Improvement Suggestions**:
**Solution 1: Add ENUM UNSIGNED Support Simultaneously**
If MySQL supports `ENUM UNSIGNED` is confirmed:
```java
static final String MYSQL_ENUM = "ENUM";
static final String MYSQL_SET = "SET";
static final String MYSQL_ENUM_UNSIGNED = "ENUM UNSIGNED"; // New addition
static final String MYSQL_SET_UNSIGNED = "SET UNSIGNED";
```
```java
case MYSQL_ENUM:
case MYSQL_ENUM_UNSIGNED: // New addition
case MYSQL_SET:
case MYSQL_SET_UNSIGNED:
builder.dataType(BasicType.STRING_TYPE);
if (typeDefine.getLength() == null || typeDefine.getLength() <= 0) {
builder.columnLength(100L);
} else {
builder.columnLength(typeDefine.getLength());
}
break;
```
**Solution 2: Add TODO Comment**
If uncertain whether ENUM UNSIGNED exists:
```java
// TODO: Verify if MySQL supports ENUM UNSIGNED type
// If yes, add MYSQL_ENUM_UNSIGNED constant and case label
case MYSQL_ENUM:
case MYSQL_SET:
case MYSQL_SET_UNSIGNED:
...
```
**Solution 3: Use Wildcard Matching (Not Recommended)**
```java
// This approach is not clear enough, not recommended
case MYSQL_ENUM:
case MYSQL_SET:
// Handle ENUM and SET with or without suffixes
if (mysqlDataType.equals("ENUM UNSIGNED") || mysqlDataType.equals("SET
UNSIGNED")) {
// fall through
}
...
```
**Rationale**:
1. Code completeness: when fixing one type, all similar types should be
considered
2. Preventive maintenance: avoid the same Issue recurring in the future
3. However, since ENUM UNSIGNED is extremely rare, it can be treated as
future work rather than requiring an immediate fix
---
### Issue 6: PR Testing Method Does Not Meet Project Requirements
**Location**: "How was this patch tested?" section in the PR description
**Related Context**:
- PR description template requires providing test steps
- Checklist requires "make sure that your code changes are covered with
tests"
**Issue Description**:
The test description in the PR description:
```
How was this patch tested?
I tested in my production env
```
This **does not satisfy** Apache SeaTunnel project testing requirements:
1. ❌ No reproducible test steps provided
2. ❌ No unit tests added
3. ❌ No E2E tests added or updated
4. ❌ "Production testing" cannot be automatically verified in CI/CD
Referencing the project's testing standards (from the PR description
template):
```
If tests were added, say they were added here.
If it was tested in a way different from regular unit tests, please clarify
how you tested step by step, ideally copy & paste-able.
```
**Potential Risks**:
- Risk 1: Modifications may not work in some cases, but CI cannot detect this
- Risk 2: Future refactoring may break this functionality
- Risk 3: Other contributors cannot verify the correctness of the
modification
**Impact Scope**:
- Direct impact: Code quality and maintainability
- Indirect impact: User production environment stability
**Severity**: MAJOR
**Improvement Suggestions**:
**Suggestion 1: Add Unit Tests (Required)**
```java
@Test
public void testConvertSetUnsigned() {
BasicTypeDefine<Object> typeDefine =
BasicTypeDefine.builder()
.name("test_set_unsigned")
.columnType("SET('a','b','c') UNSIGNED")
.dataType("SET UNSIGNED")
.length(3L)
.unsigned(true) // Mark as unsigned
.build();
Column column = MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefine);
Assertions.assertEquals("test_set_unsigned", column.getName());
Assertions.assertEquals(BasicType.STRING_TYPE, column.getDataType());
Assertions.assertEquals(3L, column.getColumnLength());
Assertions.assertEquals("SET('a','b','c') UNSIGNED",
column.getSourceType());
}
@Test
public void testConvertSetUnsignedWithNoLength() {
BasicTypeDefine<Object> typeDefine =
BasicTypeDefine.builder()
.name("test_set_unsigned_no_length")
.columnType("SET UNSIGNED")
.dataType("SET UNSIGNED")
.unsigned(true)
.build();
Column column = MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefine);
Assertions.assertEquals(BasicType.STRING_TYPE, column.getDataType());
Assertions.assertEquals(100L, column.getColumnLength()); // Default
length
}
```
**Suggestion 2: Provide Local Reproduction Steps (Recommended)**
Add to the PR description:
```
How was this patch tested?
1. Unit tests added:
- MySqlTypeConverterTest.testConvertSetUnsigned()
- MySqlTypeConverterTest.testConvertSetUnsignedWithNoLength()
2. Local verification steps:
```sql
-- Create test table
CREATE TABLE test_set_unsigned (
id INT PRIMARY KEY,
status SET('ready','processing','done') UNSIGNED
);
INSERT INTO test_set_unsigned VALUES (1, 'ready,processing');
```
Then run SeaTunnel job:
```hocon
source {
MySQL-CDC {
...
}
}
sink {
Console {
print.single = true
}
}
```
Expected behavior: Job runs successfully without type conversion errors.
3. E2E test coverage: (if added)
- Added f_set_unsigned column to AbstractMysqlCDCITBase test tables
```
**Rationale**:
1. Apache top-level projects require strict test coverage
2. Unit tests are the foundation of CI/CD automation
3. Detailed reproduction steps facilitate verification by other contributors
and users
---
### Issue 7: CHANGELOG or incompatible-changes.md Not Updated
**Location**: Documentation files in the project root directory
**Related Context**:
- The PR description Checklist mentions:
- "If necessary, please update the documentation to describe the new
feature"
- "If necessary, please update `incompatible-changes.md` to describe the
incompatibility"
**Issue Description**:
The PR modifies type conversion logic and fixes a bug (`SET UNSIGNED` type
not recognized), but:
1. ❌ `CHANGELOG.md` not updated (if this file exists)
2. ❌ `incompatible-changes.md` not updated
3. ❌ PR description states "Does this PR introduce any user-facing change?
No", but **this is a bug fix**, which is a user-visible improvement
**Potential Risks**:
- Risk 1: Users are unaware that the new version fixes this problem and may
continue to encounter the same issue
- Risk 2: Release notes lack a record of this fix
- Risk 3: Users cannot understand what changed when upgrading
**Impact Scope**:
- Direct impact: User visibility
- Indirect impact: Version release documentation
**Severity**: MINOR
**Improvement Suggestions**:
**Suggestion 1: Update Connector-V2 CHANGELOG**
Add in `docs/en/connectors/changelog/connector-jdbc.md`:
```markdown
## [Unreleased]
### Bug Fixes
* Fixed a bug where MySQL SET type with UNSIGNED attribute was not supported.
Previously, tables with `SET UNSIGNED` columns would fail with type
conversion error.
Now both `SET` and `SET UNSIGNED` are correctly mapped to SeaTunnel STRING
type.
(#10451)
```
**Suggestion 2: Correct user-facing change in PR description**
```
Does this PR introduce any user-facing change?
Yes (Bug fix)
Previous behavior:
MySQL tables with SET UNSIGNED columns would fail with type conversion error:
"Unsupported type: SET UNSIGNED"
New behavior:
SET UNSIGNED columns are correctly mapped to STRING type, same as regular
SET columns.
```
**Rationale**:
1. Bug fix is a user-visible change
2. Clear change records help users understand version differences
3. Documentation update is an Apache project best practice
---
### Issue 8: Potential Type Name Repeated Appending of " UNSIGNED" Issue
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:143-150`
**Related Context**:
- Preprocessing of `convert()` method: lines 143-150
- ZEROFILL handling: lines 144-147
- UNSIGNED appending logic: lines 148-150
**Issue Description**:
Reviewing the type name processing logic of the `convert()` method:
```java
String mysqlDataType = typeDefine.getDataType().toUpperCase();
if (mysqlDataType.endsWith("ZEROFILL")) {
mysqlDataType = mysqlDataType.substring(0, mysqlDataType.length() -
"ZEROFILL".length()).trim();
}
if (typeDefine.isUnsigned() && !(mysqlDataType.endsWith(" UNSIGNED"))) {
mysqlDataType = mysqlDataType + " UNSIGNED";
}
```
Here exists a **potential edge case**:
**Scenario Analysis**:
Assume `column.typeName()` returns `"SET UNSIGNED"` (this is the scenario
the PR aims to fix)
1. What does `typeDefine.getDataType()` return?
- In `MySqlTypeUtils.convertToSeaTunnelColumn()` (line 82):
`.dataType(column.typeName())`
- So `dataType` is also `"SET UNSIGNED"`
2. Line 143 convert to uppercase: `mysqlDataType = "SET UNSIGNED"`
3. Lines 148-150 check:
- What is the value of `typeDefine.isUnsigned()`?
- If `isUnsigned() == true` and `"SET UNSIGNED"` does not end with `"
UNSIGNED"`... but it **does** end with `" UNSIGNED"`
- So no repeated appending ✅
**But if** MySQL Connector/J behavior is inconsistent:
Scenario 1: `typeName()` returns `"SET"` but `isUnsigned()` returns `true`
- Line 148: `"SET"` does not end with `" UNSIGNED"` → append → `"SET
UNSIGNED"` ✅
Scenario 2: `typeName()` returns `"SET UNSIGNED"` but `isUnsigned()` returns
`false`
- Line 148: `"SET UNSIGNED"` ends with `" UNSIGNED"`, but `isUnsigned() ==
false` → no append → keep `"SET UNSIGNED"` ✅
Scenario 3: `typeName()` returns `"SET UNSIGNED"` and `isUnsigned()` returns
`true`
- Line 148: `"SET UNSIGNED"` ends with `" UNSIGNED"` → no append → keep
`"SET UNSIGNED"` ✅
**Conclusion**: The existing logic **appears correct**, but relies on MySQL
Connector/J behavior consistency.
**Potential Risks**:
- Risk 1: If MySQL Connector/J behaves inconsistently across different
versions, it may cause problems
- Risk 2: For other potentially duplicate types (e.g., `"INT UNSIGNED
UNSIGNED"`), there is no protection
**Impact Scope**:
- Direct impact: Accuracy of type names
- Indirect impact: switch branch matching
**Severity**: MINOR (existing logic should handle correctly)
**Improvement Suggestions**:
**Suggestion 1: Add Defensive Check**
```java
String mysqlDataType = typeDefine.getDataType().toUpperCase();
if (mysqlDataType.endsWith("ZEROFILL")) {
mysqlDataType = mysqlDataType.substring(0, mysqlDataType.length() -
"ZEROFILL".length()).trim();
}
// Remove duplicate " UNSIGNED" suffix if exists
if (mysqlDataType.endsWith(" UNSIGNED UNSIGNED")) {
log.warn("Duplicate UNSIGNED suffix detected for column: {}, type: {}",
typeDefine.getName(), mysqlDataType);
mysqlDataType = mysqlDataType.substring(0, mysqlDataType.length() - "
UNSIGNED".length());
}
if (typeDefine.isUnsigned() && !(mysqlDataType.endsWith(" UNSIGNED"))) {
mysqlDataType = mysqlDataType + " UNSIGNED";
}
```
**Suggestion 2: Add Unit Tests to Verify Edge Cases**
```java
@Test
public void testConvertSetUnsignedWithUnsignedFlag() {
// Test case where typeName = "SET UNSIGNED" and isUnsigned = true
BasicTypeDefine<Object> typeDefine =
BasicTypeDefine.builder()
.name("test")
.columnType("SET('a','b') UNSIGNED")
.dataType("SET UNSIGNED")
.length(2L)
.unsigned(true) // Explicitly set unsigned flag
.build();
Column column = MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefine);
Assertions.assertEquals(BasicType.STRING_TYPE, column.getDataType());
Assertions.assertEquals("SET('a','b') UNSIGNED", column.getSourceType());
}
@Test
public void testConvertSetWithUnsignedFlag() {
// Test case where typeName = "SET" but isUnsigned = true
BasicTypeDefine<Object> typeDefine =
BasicTypeDefine.builder()
.name("test")
.columnType("SET('a','b')")
.dataType("SET")
.length(2L)
.unsigned(true) // Set unsigned flag
.build();
Column column = MySqlTypeConverter.DEFAULT_INSTANCE.convert(typeDefine);
Assertions.assertEquals(BasicType.STRING_TYPE, column.getDataType());
// Should be converted to SET UNSIGNED
}
```
**Rationale**:
1. Defensive programming can avoid potential edge case bugs
2. Testing different scenarios ensures logic robustness
3. However, current code should already handle this correctly; this is just
an enhancement
---
--
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]
