laskoviymishka commented on code in PR #2157:
URL: https://github.com/apache/iceberg-go/pull/2157#discussion_r4241887439
##########
table/equality_delete_reader.go:
##########
@@ -1323,18 +1378,87 @@ func processEqualityDeletesColumnarForFile(ctx
context.Context, eqDeleteSets []*
}
}
+ singleInt64Sets := make([]*singleInt64EqualityDeleteSet,
len(eqDeleteSets))
+ for i, eqDel := range eqDeleteSets {
+ // Nested fields need parent validity checks and use the
generic path.
+ // Do not duplicate their encoded keys in an index that cannot
be used.
+ if len(fieldRefs[i]) == 1 && len(fieldRefs[i][0].path) == 1 {
+ singleInt64Sets[i] = eqDel.singleInt64Set(fileSchema)
+ }
+ }
+
return func(r arrow.RecordBatch) (arrow.RecordBatch, error) {
defer r.Release()
mem := compute.GetAllocator(ctx)
numRows := int(r.NumRows())
var maskBuf *memory.Buffer
+ defer func() {
+ if maskBuf != nil {
+ maskBuf.Release()
+ }
+ }()
var maskBytes []byte
+ ensureMask := func() {
Review Comment:
Since this is a perf PR, worth confirming the no-match path stays
allocation-free: `ensureMask` and the top-level `defer` both capture
`maskBuf`/`maskBytes` by reference, so both closures look like they'll
heap-escape on every batch, including the batches where nothing matches. A
`-benchmem` run on `BenchmarkProcessEqualityDeletesSingleInt64NoMatch` will say
whether it's real. If it is, a small struct with `ensure()`/`release()` methods
called directly keeps it stack-local and gets you back to the old zero-alloc
no-match path.
##########
table/equality_delete_reader.go:
##########
@@ -1323,18 +1378,87 @@ func processEqualityDeletesColumnarForFile(ctx
context.Context, eqDeleteSets []*
}
}
+ singleInt64Sets := make([]*singleInt64EqualityDeleteSet,
len(eqDeleteSets))
+ for i, eqDel := range eqDeleteSets {
+ // Nested fields need parent validity checks and use the
generic path.
+ // Do not duplicate their encoded keys in an index that cannot
be used.
+ if len(fieldRefs[i]) == 1 && len(fieldRefs[i][0].path) == 1 {
+ singleInt64Sets[i] = eqDel.singleInt64Set(fileSchema)
+ }
+ }
+
return func(r arrow.RecordBatch) (arrow.RecordBatch, error) {
defer r.Release()
mem := compute.GetAllocator(ctx)
numRows := int(r.NumRows())
var maskBuf *memory.Buffer
+ defer func() {
+ if maskBuf != nil {
+ maskBuf.Release()
+ }
+ }()
var maskBytes []byte
+ ensureMask := func() {
+ if maskBuf != nil {
+ return
+ }
+
+ maskBuf = memory.NewResizableBuffer(mem)
+
maskBuf.Resize(int(bitutil.BytesForBits(int64(numRows))))
+ maskBytes = maskBuf.Bytes()
+
+ for i := range maskBytes {
+ maskBytes[i] = 0xFF
+ }
+ }
var keyBuf bytes.Buffer
for setIdx, eqDel := range eqDeleteSets {
+ if int64Set := singleInt64Sets[setIdx]; int64Set != nil
{
Review Comment:
`len(parents) == 0` is always true here: the eligibility gate only populates
`singleInt64Sets[i]` when `len(path) == 1`, and `len(parents) == len(path)-1`.
The live concern is the fall-through: when the `*array.Int64` assertion fails,
control silently drops to the generic encoder below (re-running
`arrowArraysAtField`), and nothing in the code signals that. Correct today, but
easy to break on a later edit. I'd pull the probe into a helper returning
`handled bool` (`if handled { continue }`), which also lets you drop the dead
`parents` check.
##########
table/equality_delete_reader.go:
##########
@@ -51,12 +51,17 @@ var ErrConflictingEqualityDeleteMetadata =
errors.New("conflicting equality dele
// equalityDeleteSet holds the set of delete keys and the column names
// used to look them up in data records. Each set corresponds to one
// group of equality field IDs — delete files with different field IDs
-// produce separate sets. The set is immutable after construction so it
-// can be shared by tasks with the same delete files.
+// produce separate sets. Key material and metadata are immutable after
+// construction; derived typed indexes are built once and shared safely.
+// Typed indexes retain additional map storage alongside keys for the lifetime
Review Comment:
Good that the comment calls out the extra map storage. The one case it
doesn't mention: `mergeEqualityDeleteSets` builds a fresh `equalityDeleteSet`,
so a merged group can rebuild its own typed index on top of the per-file ones,
and the 2x can repeat per group rather than being a flat doubling.
Non-blocking, and I wouldn't try to bound it in this PR, but a sentence noting
the merged-set case would set expectations for anyone sizing this against a
large delete set.
--
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]
