hudi-agent commented on code in PR #7904:
URL: https://github.com/apache/hudi/pull/7904#discussion_r3861763104
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/io/storage/row/parquet/ParquetRowDataWriter.java:
##########
@@ -304,6 +304,7 @@ public void write(ArrayData array, int ordinal) {
}
private long timestampToInt64(TimestampData timestampData) {
+ timestampData = DateUtils.normalLocalDateToUTC(timestampData);
return utcTimestamp ? timestampData.getMillisecond() :
timestampData.toTimestamp().getTime();
Review Comment:
π€ `normalLocalDateToUTC` is applied unconditionally, but the `!utcTimestamp`
branch then calls `timestampData.toTimestamp().getTime()`, which itself applies
the localβUTC offset again β so when `write.utc-timezone=false` the offset is
subtracted twice (same issue in `timestampToInt96` at line 346). Was the
normalization meant to run only in the `utcTimestamp` branch?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/RowDataToAvroConverters.java:
##########
@@ -43,6 +43,8 @@
import java.util.List;
import java.util.Map;
+import static org.apache.hudi.util.DateUtils.normalLocalDateToUTC;
Review Comment:
π€ The new `DateUtils` class appears to be committed without a `.java`
extension (`git ls-files` shows
`hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/util/DateUtils`,
no `.java`). `javac` only compiles `*.java` files, so this class won't be
compiled and every reference to it here and in `ParquetRowDataWriter` would
fail to compile. Could you rename the file to `DateUtils.java`?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/AvroToRowDataConverters.java:
##########
@@ -206,16 +206,20 @@ private static AvroToRowDataConverter
createMapConverter(LogicalType type) {
}
private static AvroToRowDataConverter createTimestampConverter(int
precision) {
+ return createTimestampConverter(precision,false);
Review Comment:
π€ The no-arg overload hardcodes `utcTimestamp=false`, and nothing wires the
`read.utc-timezone` config into `createTimestampConverter`. So this
unconditionally switches the read conversion from
`TimestampData.fromInstant(instant)` to
`fromTimestamp(Timestamp.from(instant))` for every existing Flink reader β a
silent local-offset shift on read-back regardless of config. Is the plan to
plumb the config through here in a follow-up?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/RowDataToAvroConverters.java:
##########
@@ -166,7 +168,8 @@ public Object convert(Schema schema, Object object) {
@Override
public Object convert(Schema schema, Object object) {
- return ((TimestampData) object).toInstant().toEpochMilli();
+ // write timestamp utc timezone
+ return normalLocalDateToUTC((TimestampData)
object).toInstant().toEpochMilli();
Review Comment:
π€ nit: the comment `// write timestamp utc timezone ` just restates what
`normalLocalDateToUTC` already conveys β have you considered removing it (or
replacing it with a note explaining *why* this conversion is needed at this
specific call site)?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/RowDataToAvroConverters.java:
##########
Review Comment:
π€ **Line 182:** Only the `precision <= 3` branch got the
`normalLocalDateToUTC` normalization; this `precision <= 6` (micros) branch
still writes without it. Since Flink's default TIMESTAMP precision is 6, the
common case is left unconverted, so micros-precision tables won't get the UTC
fix and will be inconsistent with the precision-3 path. Should the micros
branch be normalized too?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1218,7 +1218,7 @@ protected void
setWriteSchemaForDeletes(HoodieTableMetaClient metaClient) {
throw new HoodieIOException("Latest commit does not have any schema
in commit metadata");
}
} else {
- throw new HoodieIOException("Deletes issued without any prior
commits");
+ LOG.warn("None rows are deleted because the table is empty");
Review Comment:
π€ This changes delete-on-empty-table from throwing `HoodieIOException` to
just logging a warning, and it lives in engine-agnostic `hudi-client-common`,
so it affects Spark and Java clients too, not only Flink. Is that intended
scope for a timezone PR? After the warn, `config.setSchema(...)` is skipped β
could you confirm the downstream delete/commit path handles an unset write
schema safely for all engines?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
--
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]
