wombatu-kun commented on code in PR #18715:
URL: https://github.com/apache/iceberg/pull/18715#discussion_r4434927566
##########
core/src/main/java/org/apache/iceberg/DataTableScan.java:
##########
@@ -88,6 +92,48 @@ public CloseableIterable<FileScanTask> doPlanFiles() {
manifestGroup = manifestGroup.planWith(planExecutor());
}
- return manifestGroup.planFiles();
+ CloseableIterable<FileScanTask> tasks = manifestGroup.planFiles();
+ Long minRowsRequested = context().minRowsRequested();
+ if (minRowsRequested != null && !shouldIgnoreResiduals()) {
Review Comment:
With Spark's default AUTO planning mode the scan builder creates a
SparkDistributedDataScan, and BaseDistributedDataScan ignores this hint even
when it plans locally, so a default Spark LIMIT query still plans the whole
table. Is that intentional, or should BaseDistributedDataScan honor the hint as
well?
##########
core/src/test/java/org/apache/iceberg/TestLocalDataTableScan.java:
##########
@@ -40,4 +50,64 @@ protected TableScan asOfTime(TableScan scan, long
timestampMillis) {
protected TableScan newScan() {
return table.newScan();
}
+
+ @TestTemplate
+ public void minRowsRequestedStopsPlanning() throws IOException {
Review Comment:
Every hint in these tests is 1 on a single manifest of one-row files, so
they still pass if the countdown decrements per file or by file size, and the
worker-pool branch never runs. Could we add a case whose hint spans two files
using the 10-row newDataFile (a hint of first.recordCount() + 1 plans exactly
two), and one that commits each file separately and expects one planned file?
##########
core/src/main/java/org/apache/iceberg/DataTableScan.java:
##########
@@ -88,6 +92,48 @@ public CloseableIterable<FileScanTask> doPlanFiles() {
manifestGroup = manifestGroup.planWith(planExecutor());
}
- return manifestGroup.planFiles();
+ CloseableIterable<FileScanTask> tasks = manifestGroup.planFiles();
+ Long minRowsRequested = context().minRowsRequested();
+ if (minRowsRequested != null && !shouldIgnoreResiduals()) {
+ return stopAfterRows(tasks, minRowsRequested);
+ }
+
+ return tasks;
+ }
+
+ private static CloseableIterable<FileScanTask> stopAfterRows(
+ CloseableIterable<FileScanTask> tasks, long minRows) {
+ return CloseableIterable.combine(
+ () ->
+ new CloseableIterator<>() {
+ private final CloseableIterator<FileScanTask> inner =
tasks.iterator();
+ private long remainingRows = minRows;
+
+ @Override
+ public boolean hasNext() {
+ return remainingRows > 0 && inner.hasNext();
Review Comment:
A negative hint now makes this scan plan no files at all, while
PlanTableScanRequest rejects the same value as invalid. Could
BaseScan.minRowsRequested reject it with a Preconditions check?
##########
core/src/test/java/org/apache/iceberg/TestLocalDataTableScan.java:
##########
@@ -40,4 +50,64 @@ protected TableScan asOfTime(TableScan scan, long
timestampMillis) {
protected TableScan newScan() {
return table.newScan();
}
+
+ @TestTemplate
+ public void minRowsRequestedStopsPlanning() throws IOException {
+
table.newFastAppend().appendFile(FILE_A).appendFile(FILE_B).appendFile(FILE_C).commit();
+
+ List<String> planned =
plannedFiles(table.newScan().minRowsRequested(FILE_A.recordCount()));
+
+ assertThat(planned).containsExactly(FILE_A.location());
+ }
+
+ @TestTemplate
+ public void filesWithDeletesDoNotCountTowardMinRowsRequested() throws
IOException {
+ assumeThat(formatVersion).isGreaterThanOrEqualTo(2);
+
table.newFastAppend().appendFile(FILE_A).appendFile(FILE_B).appendFile(FILE_C).commit();
+
table.newRowDelta().addDeletes(fileADeletes()).addDeletes(fileBDeletes()).commit();
+
+ List<String> planned =
plannedFiles(table.newScan().minRowsRequested(FILE_C.recordCount()));
+
+ assertThat(planned).containsExactly(FILE_A.location(), FILE_B.location(),
FILE_C.location());
+ }
+
+ @TestTemplate
+ public void residualFilterDisablesMinRowsRequested() throws IOException {
Review Comment:
No test covers a filter fully answered by partition values. Could we add one
with equal(bucket("data", BUCKETS_NUMBER), 0) over FILE_A, FILE_A2 and FILE_B,
where a hint of FILE_A.recordCount() plans only FILE_A?
--
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]
