Copilot commented on code in PR #10122:
URL: https://github.com/apache/seatunnel/pull/10122#discussion_r2569004948
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:
##########
@@ -317,7 +317,12 @@ public Column convert(BasicTypeDefine typeDefine) {
builder.scale(typeDefine.getScale());
break;
case MYSQL_DATETIME:
+ // MySQL DATETIME -> business time with offset
Review Comment:
This creates a semantic mismatch: MySQL DATETIME columns are now read as
OFFSET_DATE_TIME_TYPE (with system timezone), but when written back they go
through the TIMESTAMP_TZ case (line 522) which reconverts to DATETIME.
The issue is that if a user reads a DATETIME column, modifies it, and writes
it back, the timezone offset gets silently stripped during the write (since
DATETIME doesn't support timezones). This could cause unexpected behavior if
the user's system timezone changes between read and write, or if they
explicitly modify the offset.
Consider documenting this limitation or adding a warning when timezone
information might be lost during reconversion.
```suggestion
// MySQL DATETIME -> business time with offset
// WARNING: MySQL DATETIME does not store timezone
information. Mapping to OFFSET_DATE_TIME_TYPE may cause
// timezone offset to be lost when writing back. If the
system timezone changes or the offset is modified,
// unexpected behavior may occur.
log.warn("Mapping MySQL DATETIME column '{}' to
OFFSET_DATE_TIME_TYPE. MySQL DATETIME does not store timezone information. Any
timezone offset will be lost when writing back. Consider using TIMESTAMP WITH
TIME ZONE if you need to preserve timezone information.", typeDefine.getName());
```
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MysqlJdbcRowConverter.java:
##########
@@ -17,16 +17,107 @@
package org.apache.seatunnel.connectors.seatunnel.jdbc.internal.dialect.mysql;
+import org.apache.seatunnel.api.table.catalog.TableSchema;
+import org.apache.seatunnel.api.table.type.SeaTunnelDataType;
+import org.apache.seatunnel.api.table.type.SeaTunnelRow;
+import org.apache.seatunnel.api.table.type.SeaTunnelRowType;
+import org.apache.seatunnel.common.exception.CommonError;
import
org.apache.seatunnel.connectors.seatunnel.jdbc.internal.converter.AbstractJdbcRowConverter;
import
org.apache.seatunnel.connectors.seatunnel.jdbc.internal.dialect.DatabaseIdentifier;
+import org.apache.seatunnel.connectors.seatunnel.jdbc.utils.JdbcFieldTypeUtils;
+import java.sql.Date;
import java.sql.PreparedStatement;
+import java.sql.ResultSet;
import java.sql.SQLException;
+import java.sql.Timestamp;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
+import java.time.ZoneId;
+import java.util.Optional;
public class MysqlJdbcRowConverter extends AbstractJdbcRowConverter {
+ private static final ZoneId SYSTEM_ZONE = ZoneId.systemDefault();
Review Comment:
Using `ZoneId.systemDefault()` as a static field could lead to issues in
environments where the system timezone changes at runtime or in tests that
manipulate the default timezone.
Consider either:
1. Fetching `ZoneId.systemDefault()` at conversion time rather than storing
it as a static field
2. Making this configurable through connection properties
3. Adding documentation about this behavior
This is particularly important for distributed systems where different nodes
might have different system timezones, or when migrating data between
environments with different default timezones.
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:
##########
@@ -317,7 +317,12 @@ public Column convert(BasicTypeDefine typeDefine) {
builder.scale(typeDefine.getScale());
break;
case MYSQL_DATETIME:
+ // MySQL DATETIME -> business time with offset
Review Comment:
The comment here is misleading. MySQL DATETIME doesn't store timezone
information. Converting it to OFFSET_DATE_TIME_TYPE (which includes
timezone/offset information) could lead to confusion since the database doesn't
actually store that offset. The offset is inferred from the system default
timezone at read time.
Consider either:
1. Adding a comment clarifying this is an interpretation using system
default timezone
2. Documenting this behavior change in the PR description more explicitly
The comment "business time with offset" suggests the offset is stored in the
database, but MySQL DATETIME is timezone-agnostic.
```suggestion
// MySQL DATETIME does not store timezone/offset information.
// Mapping to OFFSET_DATE_TIME_TYPE: offset is inferred from
system default timezone at read time.
```
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:
##########
@@ -317,7 +317,12 @@ public Column convert(BasicTypeDefine typeDefine) {
builder.scale(typeDefine.getScale());
break;
case MYSQL_DATETIME:
+ // MySQL DATETIME -> business time with offset
+ builder.dataType(LocalTimeType.OFFSET_DATE_TIME_TYPE);
+ builder.scale(typeDefine.getScale());
+ break;
Review Comment:
**Breaking change**: This changes the behavior for ALL MySQL DATETIME
columns, not just a new TIMESTAMP_TZ type. All existing DATETIME columns will
now be read as OFFSET_DATE_TIME_TYPE (with system timezone) instead of
LOCAL_DATE_TIME_TYPE.
This is a significant breaking change that should be:
1. Clearly documented in the PR description with migration guidance
2. Potentially versioned or made opt-in via configuration
3. Tested for backward compatibility with existing pipelines
Consider whether this breaking change is intentional or if there should be a
way to distinguish between DATETIME columns that should be treated as
OFFSET_DATE_TIME_TYPE vs LOCAL_DATE_TIME_TYPE.
--
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]
