zeroshade commented on code in PR #1975:
URL: https://github.com/apache/iceberg-go/pull/1975#discussion_r3937635359
##########
table/scanner.go:
##########
@@ -923,13 +929,18 @@ func buildDVIndex(dvEntries []iceberg.ManifestEntry)
(map[string]iceberg.Manifes
// would never satisfy dataSeq <= -1 and would silently drop the DV,
// resurfacing deleted rows; an unset data sequence likewise satisfies
// -1 <= dvSeq for any known DV sequence.
-func matchDVToData(dataEntry iceberg.ManifestEntry, dvIndex
map[string]iceberg.ManifestEntry) []iceberg.DataFile {
- dvEntry, ok := dvIndex[dataEntry.DataFile().FilePath()]
+func matchDVToData(dataEntry iceberg.ManifestEntry, dvIndex
map[string]deleteFileIndexEntry) []iceberg.DataFile {
+ dataFile := dataEntry.DataFile()
+ dvEntry, ok := dvIndex[dataFile.FilePath()]
if !ok {
Review Comment:
**major** — New DV spec/partition gate silently drops deletion vectors
instead of erroring
matchDVToData now returns nil when dvEntry.file.SpecID() !=
dataFile.SpecID() or partitions differ. origin/main has no such check
(verified: `git show origin/main:table/scanner.go` sed 646-656 matches purely
on referenced_data_file + sequence number). Two problems. (a) Failure mode:
when matchDVToData returns nil, fileScanTaskForDataEntry (scanner.go:1376-1382)
falls back to posDeleteIndex.forDataFile; on a v3 DV-only table there are no
positional deletes, so the data file is planned with NO deletes and
previously-deleted rows resurface with no error and no log. A gate that can
only ever reject should surface an error, not a silent nil. (b) Inconsistency:
the equality-delete path 50 lines earlier (scanner.go:877-882) deliberately
guards `len(delPartition) > 0 && len(dataPartition) > 0` so empty partitions
apply globally; the new DV gate has no such guard, so a nil-partition DV is
dropped where a nil-partition equality delete is kept. This is also unrelated
to the PR's stated purpo
se (retaining only required stats). Calibration: I could not construct a
mismatch from iceberg-go's own writer (dv_writer.go:250 derives the DV
spec/partition from the data file), so I am NOT claiming proven live corruption
- the finding is the wrong failure mode plus unrequested scope in a perf PR.
<details><summary>Evidence</summary>
```text
Probe TestProbe_DVSpecEvolutionDropsDeletionVector: 'DV explicitly
referencing s3://bucket/data/data-001.parquet -> matched 0 delete files' ...
'Error: "[]" should have 1 item(s), but has 0'. Probe
TestProbe_DVNilPartitionVsPartitionedData: 'nil-partition DV -> matched 0
delete files'. Both DVs carry referenced_data_file naming the exact data file.
```
</details>
##########
table/equality_delete_index.go:
##########
@@ -474,13 +478,14 @@ func buildEqualityDeleteIndex(
isUnpartitioned = spec.IsUnpartitioned()
unpartitionedBySpecID[df.SpecID()] = isUnpartitioned
}
+ indexedFile := compactDeleteFileForIndex(df, partition,
df.EqualityFieldIDs())
+ indexedEntry := newEqualityDeleteIndexEntry(entry, indexedFile,
schema)
Review Comment:
**minor** — Allocating public getter used in the hot indexing loop of an
allocation-reduction PR
`compactDeleteFileForIndex(df, partition, df.EqualityFieldIDs())` calls the
public getter, which does `slices.Clone(*d.EqualityIDs)`
(manifest.go:2901-2907), allocating a fresh slice per delete file.
compactDeleteFileForIndex then immediately re-reads the same equality field IDs
through the borrowed, non-allocating accessor `dataFileCollections(file)`
(delete_file_index.go:101). Using the borrowed accessor at the call site as
well would drop one allocation per delete file, which is squarely the goal of
this PR.
##########
table/delete_file_index.go:
##########
@@ -0,0 +1,195 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "github.com/apache/iceberg-go"
+ iceberginternal "github.com/apache/iceberg-go/internal"
+)
+
+// deleteFileIndexEntry is the part of a manifest entry used after delete
+// manifests have been read. Keeping the sequence number separate lets the
+// index release the full ManifestEntry, including its unused metadata maps.
+type deleteFileIndexEntry struct {
+ file iceberg.DataFile
+ sequenceNum int64
+}
+
+// compactDeleteFileForIndex copies a delete file without retaining the wide
+// metadata maps that are not needed by scan planning or delete-file readers.
+// The returned file still carries all non-statistics metadata exposed by the
+// DataFile interface, plus only the selected statistics fields.
+//
+// Positional-delete indexes select file_pathFieldID because partition-scoped
+// position deletes use those bounds for candidate pruning. Equality-delete
+// indexes select their equality field IDs. Deletion vectors select no stats.
Review Comment:
**minor** — PR body and source comment claim DV stat trimming that the code
does not implement
The comment at delete_file_index.go:39 states 'Deletion vectors select no
stats' and the PR body states 'Deletion vectors retain no stats, but keep the
metadata needed to read the Puffin range'. Neither is true: `grep -rn
compactDeleteFileForIndex --include=*.go` shows only three call sites
(positional_delete_index.go:44,59 and equality_delete_index.go:481) - none for
DVs. buildDVIndex (scanner.go:909) stores the original unmodified
iceberg.DataFile. Consequently the `deleteEntries.dvEntries = nil` release at
scanner.go:1558 frees only the ManifestEntry wrappers while every DV's wide
stat maps stay live for the rest of planning, so the claimed DV saving does not
exist. Either wire DVs through compactDeleteFileForIndex or correct the comment
and the PR description.
##########
table/arrow_scanner.go:
##########
@@ -285,10 +285,10 @@ func sameDVBlob(a, b iceberg.DataFile) bool {
return false
}
- _, _, _, aOffset, _ := iceinternal.BorrowedDataFilePointers(a)
- _, _, _, bOffset, _ := iceinternal.BorrowedDataFilePointers(b)
+ _, _, _, aOffset, aSize := iceinternal.BorrowedDataFilePointers(a)
+ _, _, _, bOffset, bSize := iceinternal.BorrowedDataFilePointers(b)
- return *aOffset == *bOffset
+ return *aOffset == *bOffset && *aSize == *bSize
}
Review Comment:
**minor** — Unrelated behavior changes bundled into a perf PR
Four observable-behavior changes ride along with the stat-trimming work:
sameDVBlob now also compares content_size (arrow_scanner.go:288-291);
buildDVIndex now hard-errors on a missing/empty referenced_data_file where main
silently skipped the entry (scanner.go:906); filePathMayMatch now skips pruning
entirely when bounds are inverted (positional_delete_index.go:155-163); and
appendDeletionVectorRows / readAllDeletionVectors now reject empty-string refs.
Each is individually defensible - the inverted-bounds change is conservative
and the empty-ref checks close a real hole - but none is stat retention, and
buildDVIndex's new error turns a table that previously scanned into a hard scan
failure. Splitting these into their own PR would let them be reviewed on their
own merits and keep this diff's perf claim auditable. I verified sameDVBlob
cannot nil-deref: readAllDeletionVectors nil-checks contentOffset/contentSize
(arrow_scanner.go:173) before the uniqueDVs lookup that reaches sameD
VBlob.
--
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]
