Copilot commented on code in PR #1405:
URL: https://github.com/apache/fluss/pull/1405#discussion_r2234763388
##########
fluss-lake/fluss-lake-paimon/src/main/java/com/alibaba/fluss/lake/paimon/tiering/PaimonLakeCommitter.java:
##########
@@ -74,6 +71,14 @@ public PaimonCommittable
toCommittable(List<PaimonWriteResult> paimonWriteResult
for (PaimonWriteResult paimonWriteResult : paimonWriteResults) {
committable.addFileCommittable(paimonWriteResult.commitMessage());
}
+ if (!paimonWriteResults.isEmpty()) {
+ committable.addProperty(
+ FLUSS_LAKE_SNAP_BUCKET_OFFSET_PROPERTY,
+ mapper.writeValueAsString(
+ paimonWriteResults.stream()
+ .map(PaimonWriteResult::bucketOffset)
+ .collect(Collectors.toList())));
Review Comment:
[nitpick] Using .collect(Collectors.toList()) is unnecessary here since the
mapper can serialize the stream directly. Consider using .toList() for better
performance and readability.
##########
fluss-lake/fluss-lake-paimon/src/test/java/com/alibaba/fluss/lake/paimon/tiering/PaimonTieringTest.java:
##########
@@ -129,13 +129,22 @@ void testTieringWriteTable(boolean isPrimaryKeyTable,
boolean isPartitioned) thr
}
Map<Tuple2<String, Integer>, List<LogRecord>> recordsByBucket = new
HashMap<>();
- List<String> partitions =
- isPartitioned ? Arrays.asList("p1", "p2", "p3") :
Collections.singletonList(null);
+ Map<Long, String> partitionIdAndName =
+ isPartitioned
+ ? new HashMap<Long, String>() {
+ {
+ put(1L, "p1");
+ put(2L, "p2");
+ put(3L, "p3");
+ }
+ }
+ : Collections.singletonMap(null, null);
Review Comment:
[nitpick] Using Collections.singletonMap(null, null) creates a map with a
null key and null value, which can be confusing. Consider using a more explicit
approach like Map.of() with a clear comment or a different structure for the
non-partitioned case.
```suggestion
: Map.of(); // Use an empty map to represent the
non-partitioned case
```
##########
fluss-lake/fluss-lake-paimon/src/main/java/com/alibaba/fluss/lake/paimon/tiering/PaimonBucketOffset.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.alibaba.fluss.lake.paimon.tiering;
+
+import
com.alibaba.fluss.shaded.jackson2.com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import
com.alibaba.fluss.shaded.jackson2.com.fasterxml.jackson.annotation.JsonInclude;
+
+import javax.annotation.Nullable;
+
+import java.io.Serializable;
+
+/** The bucket offset information to be stored in Paimon's snapshot property.
*/
+@JsonInclude(JsonInclude.Include.NON_NULL)
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class PaimonBucketOffset implements Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private long logOffset;
+ private int bucket;
+ private @Nullable Long partitionId;
+ private @Nullable String partitionName;
+
Review Comment:
[nitpick] The default constructor should have a comment explaining its
purpose, especially since it's likely required for JSON deserialization given
the Jackson annotations.
```suggestion
// Default constructor required for JSON deserialization by Jackson.
```
--
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]
