Copilot commented on code in PR #4312:
URL: https://github.com/apache/flink-cdc/pull/4312#discussion_r2917389397
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-doris/src/main/java/org/apache/flink/cdc/connectors/doris/sink/DorisRowConverter.java:
##########
@@ -117,7 +117,10 @@ static SerializationConverter
createExternalConverter(DataType type, ZoneId pipe
final int zonedP = ((ZonedTimestampType) type).getPrecision();
return (index, val) -> val.getTimestamp(index,
zonedP).toTimestamp();
case TIME_WITHOUT_TIME_ZONE:
- return (index, val) -> val.getTime(index).toLocalTime();
+ return (index, val) ->
+ val.getTime(index)
+ .toLocalTime()
+ .format(DorisEventSerializer.TIME_FORMATTER);
Review Comment:
The conversion currently forces a millisecond suffix for all TIME values via
`HH:mm:ss.SSS`, so `TIME(0)` becomes `HH:mm:ss.000`. This changes the textual
representation compared to typical `TIME(0)` formatting and ignores the
declared TIME precision. Consider formatting based on the `TimeType` precision
(e.g., `HH:mm:ss` for precision 0; fractional seconds only when precision > 0,
padded to the precision), or use an optional-fraction formatter to avoid
emitting `.000` when not needed.
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-doris/src/main/java/org/apache/flink/cdc/connectors/doris/sink/DorisEventSerializer.java:
##########
@@ -20,6 +20,7 @@
import org.apache.flink.api.java.tuple.Tuple2;
import org.apache.flink.cdc.common.configuration.Configuration;
import org.apache.flink.cdc.common.data.RecordData;
+import org.apache.flink.cdc.common.data.TimeData;
Review Comment:
`TimeData` appears to be imported only for the Javadoc link. Many build
setups enforce no-unused-imports via Checkstyle/Spotless, and Javadoc
references typically don’t count as usage. To avoid a style failure, remove the
import and reference the class in Javadoc with its fully-qualified name (or
otherwise ensure the import is used in code).
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-doris/src/main/java/org/apache/flink/cdc/connectors/doris/sink/DorisRowConverter.java:
##########
@@ -117,7 +117,10 @@ static SerializationConverter
createExternalConverter(DataType type, ZoneId pipe
final int zonedP = ((ZonedTimestampType) type).getPrecision();
return (index, val) -> val.getTimestamp(index,
zonedP).toTimestamp();
case TIME_WITHOUT_TIME_ZONE:
- return (index, val) -> val.getTime(index).toLocalTime();
+ return (index, val) ->
+ val.getTime(index)
+ .toLocalTime()
+ .format(DorisEventSerializer.TIME_FORMATTER);
Review Comment:
`DorisRowConverter` depending on `DorisEventSerializer.TIME_FORMATTER`
couples conversion logic to the serializer class. To reduce cross-class
coupling, consider moving TIME formatting constants/utilities into a dedicated
shared utility (or into `DorisRowConverter` if only used there), keeping
`DorisEventSerializer` focused on event serialization.
--
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]
