laskoviymishka commented on code in PR #2106:
URL: https://github.com/apache/iceberg-go/pull/2106#discussion_r4232306341
##########
table/scanner.go:
##########
@@ -2234,17 +2265,18 @@ func (scan *Scan) ReadTasks(ctx context.Context, tasks
[]FileScanTask) (*arrow.S
}
outSchema, records, err := (&arrowScan{
- metadata: scan.metadata,
- fs: fs,
- scanSchema: effectiveSchema,
- projectedSchema: schema,
- boundRowFilter: boundFilter,
- filterSchema: effectiveSchema,
- caseSensitive: scan.caseSensitive,
- rowLimit: scan.limit,
- options: scan.options,
- concurrency: scan.concurrency,
- arrowBatchSize: scan.arrowBatchSize,
+ metadata: scan.metadata,
+ fs: fs,
+ scanSchema: effectiveSchema,
+ projectedSchema: schema,
+ boundRowFilter: boundFilter,
+ filterSchema: effectiveSchema,
+ caseSensitive: scan.caseSensitive,
+ taskResidualsBound: true,
Review Comment:
The wrong-slice bind is pinned now:
`TestReadTasksPassesBoundResidualsToGetRecords` would error or leak id 1 if an
unbound residual reached the reader through the wrong slice. A reverted clone
still isn't pinned, though. If someone drops the lazy clone and goes back to an
unconditional `slices.Clone(tasks)` in `ReadTasks`, or stops calling
`bindReadTasksResiduals`, every test stays green, because the only identity
assertion lives on the helper one call down, not on what `ReadTasks` actually
hands `GetRecords` here.
This is the boundary from rounds 1 and 3, and it's the one thing I'd want
biting before merge. Either the reader seam or the
real-parquet-with-allocation-check from last round works; both observe the
slice at this call site.
##########
table/scanner.go:
##########
@@ -2161,6 +2195,12 @@ func (scan *Scan) ToArrowRecords(ctx context.Context)
(*arrow.Schema, iter.Seq2[
// reached; if no such task is processed, the file is not read and its error
is not
// returned. The returned iterator is single-use.
//
+// The caller must not mutate tasks or any task element while the returned
+// iterator can still be used, until a range over it has returned or the
iterator
Review Comment:
The nested-slices note landed, thanks. This sentence still has the problem
"abandoned" did: "can still be used" and "otherwise dropped" aren't events a Go
caller can observe, so it doesn't tell them when it's actually safe to touch
`tasks` again. I'd phrase it around ranging instead, something like: "do not
modify tasks or its elements after calling ReadTasks until you've finished
ranging over the returned iterator, including after breaking out early; if you
never range over it, don't modify tasks until you're sure it won't be used."
##########
table/arrow_scanner.go:
##########
@@ -1235,14 +1235,15 @@ type arrowScan struct {
// rowGroupFilter is used only for Parquet statistics and bloom-filter
// pruning. It lets callers keep boundRowFilter as AlwaysTrue while they
// must evaluate the real row filter after position-dependent
enrichment.
- rowGroupFilter iceberg.BooleanExpression
- filterSchema *iceberg.Schema
- caseSensitive bool
- rowLimit int64
- options iceberg.Properties
- filterPlanCache compiledFileFilterPlanCache
- fileReadPlanCache preparedFileReadPlanCache
- cacheFileReadPlan bool
+ rowGroupFilter iceberg.BooleanExpression
+ filterSchema *iceberg.Schema
+ caseSensitive bool
+ rowLimit int64
+ options iceberg.Properties
+ filterPlanCache compiledFileFilterPlanCache
+ fileReadPlanCache preparedFileReadPlanCache
+ cacheFileReadPlan bool
+ taskResidualsBound bool
Review Comment:
A line on this field would be worth it. With it set, `rowFilterForTask`
returns `task.Residual` with no validation, so the whole thing rests on the
invariant that every non-nil residual reaching `GetRecords` was already bound
and validated against `filterSchema`, which today only the `ReadTasks` +
`bindReadTasksResiduals` path guarantees, and only by convention. A future
caller that sets the flag without going through that path silently drops
validation. Something like: "true when every non-nil FileScanTask.Residual
passed to GetRecords is already bound and validated against filterSchema;
rowFilterForTask then skips per-task binding."
--
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]
