Revanth14 opened a new pull request, #2430:
URL: https://github.com/apache/iceberg-go/pull/2430
### Problem
`ScanReport` counts manifests and results but not the entries inside scanned
manifests that the partition or inclusive metrics evaluator rejected, so a
report can't show file-level pruning. #1501 left this as a documented follow-up
in `table/scan_metrics.go`; the wire fields and OTel mapping already exist.
Closes #2429.
### Fix
Each per-manifest worker wraps its partition and metrics evaluators with
`countRejections`, counting into a worker-local `int64` that the planner merges
into the accumulator after `g.Wait()`. No atomics, same pattern as #2149. Data
manifests feed `skipped-data-files` and delete manifests feed
`skipped-delete-files`, matching Java's `ManifestReader.entries()`. Deleted
entries are dropped before the evaluators in table scans, so they aren't
counted, and entries in pruned manifests stay in `skipped-data-manifests` only.
Both counters are emitted as `0` when nothing is pruned, as Java does.
`indexed-delete-files` stays a follow-up.
The incremental append and changelog scans report the same evaluator
rejections. The issue proposed also counting the changelog scan's
`discardExisting` skip for parity with Java's `ignoreExisting` counter, but
Java's incremental scans never attach `ScanMetrics` to their `ManifestGroup`
(it defaults to `ScanMetrics.noop()`), so that counter never reaches a report.
I kept the metric to filter rejections only.
### Testing
`scan_skipped_files_test.go` builds a table partitioned by `category`: one
data manifest with 3 categories × 4 files with known `id` bounds plus a DELETED
entry, a second data manifest that manifest pruning drops, and a delete
manifest with one position delete per category. With `category == 1 AND id >=
25`, `PlanFiles` reports 10 skipped data files (8 by partition, 2 by bounds), 2
results, 2 skipped delete files and 1 skipped data manifest; with no filter
both counters are `0`. The incremental append scan on the same table and the
changelog scan on the existing changelog fixture are covered too. Breaking each
merge point and the increment fails at least one of these tests. `go test -race
./...` passes.
`BenchmarkPlanDataManifestTasks/.../streamed`, before and after interleaved
over 10 rounds (Apple M1 Pro): no significant time change on any workload (all
p ≥ 0.1, geomean −1.5%). Allocations rise by a few per manifest for the
wrappers and counter (+0.03%); bytes are unchanged.
```
│ before │ after vs
base
always_true/manifests=8/entries=1000/streamed 3.795m 3.660m ~
(p=0.393 n=10)
real_predicate/manifests=8/entries=1000/streamed 4.257m 4.151m ~
(p=0.971 n=10)
always_true/manifests=32/entries=1000/streamed 13.10m 12.83m ~
(p=0.105 n=10)
real_predicate/manifests=32/entries=1000/streamed 15.46m 15.10m ~
(p=0.393 n=10)
always_true/manifests=8/entries=10000/streamed 25.09m 25.18m ~
(p=0.796 n=10)
real_predicate/manifests=8/entries=10000/streamed 30.36m 30.67m ~
(p=0.796 n=10)
geomean 11.64m 11.47m -1.52%
```
### Context
Follow-up to #1501 (proposal #1236), part of #485. Found while comparing
scan reports on Google's Lakehouse public datasets, where a 3,001-file table
reports 147 result files but not how many of the scanned manifest's entries
were pruned.
--
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]
