Copilot commented on code in PR #1565:
URL: https://github.com/apache/fluss/pull/1565#discussion_r2284432405
##########
fluss-lake/fluss-lake-iceberg/src/main/java/com/alibaba/fluss/lake/iceberg/utils/IcebergConversions.java:
##########
@@ -28,4 +33,20 @@ public class IcebergConversions {
public static TableIdentifier toIceberg(TablePath tablePath) {
return TableIdentifier.of(tablePath.getDatabaseName(),
tablePath.getTableName());
}
+
+ public static PartitionKey toPartition(Table table, WriterInitContext
writerInitContext) {
+ PartitionSpec partitionSpec = table.spec();
+ Schema schema = table.schema();
+ PartitionKey partitionKey = new PartitionKey(partitionSpec, schema);
+ int pos = 0;
+ String partitionName = writerInitContext.partition();
+ if (partitionName != null) {
+ String[] partitionArr = partitionName.split("\\$");
Review Comment:
[nitpick] The partition delimiter '$' is a magic string that should be
defined as a constant. This improves maintainability and makes the parsing
logic more explicit.
```suggestion
String[] partitionArr = partitionName.split("\\" +
PARTITION_DELIMITER);
```
##########
fluss-lake/fluss-lake-iceberg/src/test/java/com/alibaba/fluss/lake/iceberg/IcebergTieringTest.java:
##########
@@ -343,6 +400,10 @@ private void verifyTableRecords(
.isEqualTo(expectRecord.getRow().getString(1).toString());
assertThat(actualRecord.get(2, String.class))
.isEqualTo(expectRecord.getRow().getString(2).toString());
+ if (partition != null) {
+ assertThat(actualRecord.get(2,
String.class)).isEqualTo(partition);
Review Comment:
This verification duplicates the assertion on line 401-402. The same field
(index 2) is being verified twice - once against the expected record data and
once against the partition name, which creates conflicting assertions for
partitioned tables.
```suggestion
if (partition != null) {
assertThat(actualRecord.get(2,
String.class)).isEqualTo(partition);
} else {
assertThat(actualRecord.get(2, String.class))
.isEqualTo(expectRecord.getRow().getString(2).toString());
```
##########
fluss-lake/fluss-lake-paimon/src/test/java/com/alibaba/fluss/lake/paimon/tiering/PaimonTieringTest.java:
##########
@@ -473,26 +431,16 @@ private void verifyPrimaryKeyTableRecord(
assertThat(actualRow.getString(1).toString())
.isEqualTo(expectRecord.getRow().getString(1).toString());
+ assertThat(actualRow.getString(2).toString())
+ .isEqualTo(expectRecord.getRow().getString(2).toString());
if (partition != null) {
- // For partitioned tables, partition field should match record
data
- assertThat(actualRow.getString(2).toString())
-
.isEqualTo(expectRecord.getRow().getString(2).toString());
- // check system columns: __bucket, __offset, __timestamp
- assertThat(actualRow.getInt(3)).isEqualTo(expectBucket);
-
assertThat(actualRow.getLong(4)).isEqualTo(expectRecord.logOffset());
- long actualTimestamp = actualRow.getTimestamp(5,
6).getMillisecond();
- long expectedTimestamp = expectRecord.timestamp();
- assertThat(Math.abs(actualTimestamp -
expectedTimestamp)).isLessThanOrEqualTo(10L);
- } else {
- // For non-partitioned tables
- assertThat(actualRow.getString(2).toString())
-
.isEqualTo(expectRecord.getRow().getString(2).toString());
- // check system columns: __bucket, __offset, __timestamp
- assertThat(actualRow.getInt(3)).isEqualTo(expectBucket);
-
assertThat(actualRow.getLong(4)).isEqualTo(expectRecord.logOffset());
- assertThat(actualRow.getTimestamp(5, 6).getMillisecond())
- .isEqualTo(expectRecord.timestamp());
+
assertThat(actualRow.getString(2).toString()).isEqualTo(partition);
Review Comment:
There's a logical error in the verification. Line 434-435 verifies that
column 2 matches the expected record data, but then line 437 immediately
overwrites this verification for partitioned tables, asserting that the same
column should equal the partition name. This creates conflicting assertions for
the same field.
```suggestion
if (partition != null) {
assertThat(actualRow.getString(2).toString()).isEqualTo(partition);
} else {
assertThat(actualRow.getString(2).toString())
.isEqualTo(expectRecord.getRow().getString(2).toString());
```
--
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]
