github-actions[bot] commented on code in PR #61112:
URL: https://github.com/apache/doris/pull/61112#discussion_r2900558334
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTransaction.java:
##########
@@ -73,6 +73,7 @@ public class IcebergTransaction implements Transaction {
private String branchName;
// Rewrite operation support
+ long startingSnapshotId = -1L; // Track the starting snapshot ID for
rewrite operations
private final List<DataFile> filesToDelete = Lists.newArrayList();
Review Comment:
**Missing `private` modifier.** All other fields in this class are declared
`private`, but `startingSnapshotId` is package-private (no access modifier).
This appears to be an oversight.
```suggestion
private long startingSnapshotId = -1L; // Track the starting snapshot ID
for rewrite operations
```
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTransaction.java:
##########
@@ -73,6 +73,7 @@ public class IcebergTransaction implements Transaction {
private String branchName;
// Rewrite operation support
+ long startingSnapshotId = -1L; // Track the starting snapshot ID for
rewrite operations
private final List<DataFile> filesToDelete = Lists.newArrayList();
private final List<DataFile> filesToAdd = Lists.newArrayList();
Review Comment:
**Potential `NullPointerException`.** `table.currentSnapshot()` returns
`null` for a table with no snapshots. Every other non-test caller of
`currentSnapshot()` in this codebase explicitly handles the null case (e.g.,
`IcebergRewriteDataFilesAction.java:172`,
`IcebergRewriteManifestsAction.java:72`, `IcebergUtils.java:948`, etc.).
Although the current call path guards against this in
`IcebergRewriteDataFilesAction.executeAction()` (line 172), `beginRewrite()` is
a public method — any future caller could invoke it on an empty table. Also,
note that the upstream null check and this code operate on **different**
`Table` instances (each calls `IcebergUtils.getIcebergTable()` independently),
creating a TOCTOU gap.
Suggested fix:
```java
Snapshot currentSnapshot = table.currentSnapshot();
Preconditions.checkNotNull(currentSnapshot,
"Cannot begin rewrite: table %s has no snapshots", dorisTable.getName());
this.startingSnapshotId = currentSnapshot.snapshotId();
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
