Copilot commented on code in PR #2842:
URL: https://github.com/apache/fluss/pull/2842#discussion_r2916583702
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/source/IcebergSplitPlanner.java:
##########
@@ -68,9 +68,9 @@ public List<IcebergSplit> plan() throws IOException {
Table table = catalog.loadTable(toIceberg(tablePath));
Function<FileScanTask, List<String>> partitionExtract =
createPartitionExtractor(table);
Function<FileScanTask, Integer> bucketExtractor =
createBucketExtractor(table);
- TableScan tableScan =
table.newScan().useSnapshot(snapshotId).includeColumnStats();
+ TableScan tableScan = table.newScan().useSnapshot(snapshotId);
if (filter != null) {
- tableScan = tableScan.filter(filter);
+ tableScan = tableScan.includeColumnStats().filter(filter);
}
Review Comment:
This change alters planning behavior by conditionally enabling
`includeColumnStats()` only when `filter != null`, but there is no test
asserting the new contract (stats absent when no filter; stats present when
filter exists). Please add/extend an Iceberg source test to cover both paths so
the intended performance optimization doesn’t regress across Iceberg version
changes (e.g., assert presence/absence of file metrics like `lowerBounds()` on
planned tasks for the same table with/without a filter).
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/source/IcebergSplitPlanner.java:
##########
@@ -68,9 +68,9 @@ public List<IcebergSplit> plan() throws IOException {
Table table = catalog.loadTable(toIceberg(tablePath));
Function<FileScanTask, List<String>> partitionExtract =
createPartitionExtractor(table);
Function<FileScanTask, Integer> bucketExtractor =
createBucketExtractor(table);
- TableScan tableScan =
table.newScan().useSnapshot(snapshotId).includeColumnStats();
+ TableScan tableScan = table.newScan().useSnapshot(snapshotId);
if (filter != null) {
Review Comment:
The PR description still contains the default template and does not state
the purpose, linked issue, or tests run. Please update the PR description
(e.g., link the issue being fixed and list the specific UT/IT that validate
this change) so reviewers can verify intent and coverage.
--
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]
