zeroshade commented on code in PR #1937:
URL: https://github.com/apache/iceberg-go/pull/1937#discussion_r3936147396
##########
table/arrow_scanner.go:
##########
@@ -661,6 +704,121 @@ func readDeletes(ctx context.Context, fs iceio.IO,
dataFile iceberg.DataFile) (_
return acc.finish(), nil
}
+func newPositionDeleteRowGroupTester(schema *arrow.Schema, targets
map[string]struct{}) (*tblutils.ParquetRowGroupTester, error) {
+ if len(targets) == 0 || len(targets) > inPredicateLimit {
+ return nil, nil
Review Comment:
**minor** — Field-ID validation is skipped by the early return, so the same
corrupt file errors or reads depending on query shape
newPositionDeleteRowGroupTester returns (nil, nil) before calling
positionDeletePruningEnabled whenever len(targets)==0 or
len(targets)>inPredicateLimit. A delete file with swapped or duplicated
reserved field IDs therefore fails with ErrInvalidSchema when a query touches
1..200 data files, but reads successfully when it touches >200, or when any
task lacks a usable path and the nil whole-file fallback kicks in. Neither path
is unsafe (validation only gates pushdown, and the unvalidated paths do no
pruning), so this is a consistency/support concern rather than a correctness
one — but a scan that fails only for some query shapes is hard to diagnose.
Consider validating unconditionally, or documenting that validation is
deliberately scoped to the pushdown path.
##########
table/arrow_scanner.go:
##########
@@ -79,16 +80,35 @@ func releasePerFilePosDeletes(deletesPerFile
perFilePosDeletes) {
func readAllDeleteFiles(ctx context.Context, fs iceio.IO, tasks
[]FileScanTask, concurrency int) (perFilePosDeletes, error) {
deletesPerFile := make(perFilePosDeletes)
uniqueDeletes := make(map[string]iceberg.DataFile)
+ targetsByDelete := make(perDeleteFileTargets)
for _, t := range tasks {
for _, d := range t.DeleteFiles {
if d.ContentType() != iceberg.EntryContentPosDeletes {
continue
}
- if _, ok := uniqueDeletes[d.FilePath()]; !ok {
- uniqueDeletes[d.FilePath()] = d
+ deletePath := d.FilePath()
+ if _, ok := uniqueDeletes[deletePath]; !ok {
+ uniqueDeletes[deletePath] = d
}
+
+ targets, ok := targetsByDelete[deletePath]
+ if !ok {
+ targets = make(map[string]struct{})
+ targetsByDelete[deletePath] = targets
+ }
+ // A nil target set means that at least one task did
not carry a
+ // usable data-file path. Keep the old whole-file read
in that case.
+ if targets == nil {
+ continue
+ }
+ if t.File == nil || t.File.FilePath() == "" {
+ targetsByDelete[deletePath] = nil
+
+ continue
+ }
+ targets[t.File.FilePath()] = struct{}{}
}
Review Comment:
**minor** — Filtering also drops deletes from a delete file not assigned to
the task — a semantic change, not just perf
The target set is built only from tasks that reference a given delete file,
so rows in that file addressing a data file whose task did not list it are now
discarded. Previously the whole file was read and those rows landed in
deletesPerFile, where the other task's lookup would apply them. I believe the
new behaviour is the spec-correct one (delete-to-data assignment is the
planner's job, and applying an unassigned delete is over-deletion), and I
confirmed no under-deletion is possible because targets always contains
t.File.FilePath() for every task that references the file. Still, the PR is
framed as pure perf with an explicit 'keeps the existing result shape' claim,
and this changes observable output. Worth a sentence in the PR body and a
regression test pinning the intended semantics.
--
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]
