daguimu commented on code in PR #28071:
URL: https://github.com/apache/flink/pull/28071#discussion_r3203286843
##########
flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/RowDataToAvroConverters.java:
##########
@@ -166,7 +168,11 @@ public Object convert(Schema schema, Object object) {
@Override
public Object convert(Schema schema, Object
object) {
- return ((TimestampData)
object).toInstant().toEpochMilli();
+ final TimestampData timestampData =
(TimestampData) object;
+ if (isMicrosLogicalType(schema)) {
Review Comment:
Apologies — I need to correct my earlier comment. I claimed
`AvroToRowDataConverters` already honors `timestamp-micros`, but on closer
reading it does not: `convertToTimestamp` treats every incoming `Long` as
milliseconds regardless of the schema's logical type, and there's no
`Conversions.addLogicalTypeConversion` registered anywhere in flink-avro.
That means today both read and write ignore the logical type symmetrically,
which is why a Flink-internal round-trip with a `timestamp-micros` schema
happens to round-trip correctly (two bugs cancel out). Landing only the
write-side fix in this PR would break that internal round-trip — exactly the
regression you flagged. Your concern was well-founded; I had it wrong.
I'll expand the scope of this PR to also fix the read path so the bug is
addressed symmetrically. That removes the regression risk and makes a config
flag unnecessary. Will push a follow-up commit and update the PR title
accordingly.
--
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]
