Copilot commented on code in PR #2620:
URL: https://github.com/apache/fluss/pull/2620#discussion_r2785346873
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/source/IcebergRecordAsFlussRow.java:
##########
@@ -86,6 +88,14 @@ public short getShort(int pos) {
@Override
public int getInt(int pos) {
Object value = icebergRecord.get(pos);
+ // Iceberg returns LocalDate for DATE columns and LocalTime for TIME
columns,
+ // but Fluss InternalRow uses getInt() for both (epoch days and
millis-of-day).
+ if (value instanceof LocalDate) {
+ return (int) ((LocalDate) value).toEpochDay();
+ }
+ if (value instanceof LocalTime) {
+ return (int) ((LocalTime) value).toNanoOfDay() / 1_000_000;
Review Comment:
In `getInt`, the `LocalTime` conversion currently casts to `int` before
dividing by 1_000_000 due to Java operator precedence (`(int) longExpr /
1_000_000`). This will overflow for most times (nanos-of-day doesn't fit in an
int) and produce incorrect millis-of-day. Divide on the `long` first, then cast
the result to `int`.
```suggestion
return (int) (((LocalTime) value).toNanoOfDay() / 1_000_000);
```
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/source/IcebergArrayAsFlussArray.java:
##########
@@ -72,7 +74,14 @@ public short getShort(int pos) {
@Override
public int getInt(int pos) {
- return (Integer) icebergList.get(pos);
+ Object value = icebergList.get(pos);
+ if (value instanceof LocalDate) {
+ return (int) ((LocalDate) value).toEpochDay();
+ }
+ if (value instanceof LocalTime) {
+ return (int) ((LocalTime) value).toNanoOfDay() / 1_000_000;
Review Comment:
Same overflow/precedence issue as in `IcebergRecordAsFlussRow`: `return
(int) ((LocalTime) value).toNanoOfDay() / 1_000_000;` casts before dividing,
producing incorrect millis-of-day. Compute the division on `long` and only then
cast to `int`.
```suggestion
return (int) (((LocalTime) value).toNanoOfDay() / 1_000_000L);
```
##########
fluss-lake/fluss-lake-iceberg/src/test/java/org/apache/fluss/lake/iceberg/source/IcebergRecordAsFlussRowTest.java:
##########
@@ -170,8 +182,33 @@ void testAllDataTypes() {
assertThat(icebergRecordAsFlussRow.getChar(12, 10).toString())
.isEqualTo("Hello"); // char_data
- // Test field count (excluding system columns)
- assertThat(icebergRecordAsFlussRow.getFieldCount()).isEqualTo(15);
+ assertThat(icebergRecordAsFlussRow.getFieldCount()).isEqualTo(19);
+ }
+
+ @Test
+ void testGetIntWithLocalDateAndLocalTime() {
+ LocalDate date = LocalDate.of(2020, 6, 15);
+ LocalTime time = LocalTime.of(14, 30, 0);
+
+ record.setField("id", 1L);
+ record.setField("date_field", date);
+ record.setField("time_field", time);
+ record.setField("date_array", Arrays.asList(date));
+ record.setField("time_array", Arrays.asList(time));
+ record.setField("__bucket", 1);
+ record.setField("__offset", 100L);
+ record.setField("__timestamp", OffsetDateTime.now(ZoneOffset.UTC));
+
+ icebergRecordAsFlussRow.replaceIcebergRecord(record);
+
+ assertThat(icebergRecordAsFlussRow.getInt(15)).isEqualTo((int)
date.toEpochDay());
+ assertThat(icebergRecordAsFlussRow.getInt(16))
+ .isEqualTo((int) time.toNanoOfDay() / 1_000_000);
+
+ InternalArray dateArray = icebergRecordAsFlussRow.getArray(17);
+ InternalArray timeArray = icebergRecordAsFlussRow.getArray(18);
+ assertThat(dateArray.getInt(0)).isEqualTo((int) date.toEpochDay());
+ assertThat(timeArray.getInt(0)).isEqualTo((int) time.toNanoOfDay() /
1_000_000);
Review Comment:
The expected millis-of-day is computed with `(int) time.toNanoOfDay() /
1_000_000`, which has the same precedence/overflow problem as the production
code (casts before dividing). This makes the test pass even when the
implementation is wrong. Compute expected values by dividing on `long` first
and then casting to `int`, so the test would catch the overflow bug.
```suggestion
.isEqualTo((int) (time.toNanoOfDay() / 1_000_000L));
InternalArray dateArray = icebergRecordAsFlussRow.getArray(17);
InternalArray timeArray = icebergRecordAsFlussRow.getArray(18);
assertThat(dateArray.getInt(0)).isEqualTo((int) date.toEpochDay());
assertThat(timeArray.getInt(0)).isEqualTo((int) (time.toNanoOfDay()
/ 1_000_000L));
```
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/source/IcebergArrayAsFlussArray.java:
##########
@@ -72,7 +74,14 @@ public short getShort(int pos) {
@Override
public int getInt(int pos) {
- return (Integer) icebergList.get(pos);
+ Object value = icebergList.get(pos);
+ if (value instanceof LocalDate) {
+ return (int) ((LocalDate) value).toEpochDay();
+ }
+ if (value instanceof LocalTime) {
+ return (int) ((LocalTime) value).toNanoOfDay() / 1_000_000;
+ }
+ return (Integer) value;
}
Review Comment:
`IcebergArrayAsFlussArray` now supports `LocalDate`/`LocalTime` in `getInt`,
but `toIntArray()` below still does `(int) icebergList.get(i)`, which will fail
for date/time arrays (e.g., `InternalRowUtils.copyArray` calls `toIntArray()`
for DATE/TIME_WITHOUT_TIME_ZONE when non-nullable). Consider updating
`toIntArray()` to use the same conversions as `getInt`.
--
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]
