laskoviymishka commented on code in PR #1382:
URL: https://github.com/apache/iceberg-go/pull/1382#discussion_r3560308114
##########
table/scan_planning_test.go:
##########
@@ -107,11 +129,16 @@ type fakeScanPlanner struct {
result ScanPlanningResult
supports bool
err error
+ // captured after PlanFiles receives it
+ receivedIdentifier Identifier
}
func (f *fakeScanPlanner) SupportsRemoteScanPlanning() bool { return
f.supports }
-func (f *fakeScanPlanner) PlanFiles(context.Context, ScanPlanningRequest)
(ScanPlanningResult, error) {
+func (f *fakeScanPlanner) PlanFiles(_ context.Context, req
ScanPlanningRequest) (ScanPlanningResult, error) {
+ // Capture a defensive copy to verify call-site identifiers are not
shared.
+ f.receivedIdentifier = append([]string(nil), req.Identifier...)
+
Review Comment:
This test can't fail. The fake planner copies `req.Identifier` into its own
backing array before storing it (`append([]string(nil), ...)`), so mutating
`planReq.receivedIdentifier[0]` only touches that copy — it can never reach
`scan.identifier` whether or not `planFilesRemote` clones. Remove the
production clone at `scanner.go` and this stays green.
I'd store the slice as-is instead — `f.receivedIdentifier = req.Identifier`
— so the captured slice aliases whatever the call site passed. Then mutating it
corrupts `scan.identifier` iff `planFilesRemote` didn't clone, which is the
property we actually want to assert.
##########
table/table.go:
##########
@@ -851,7 +853,7 @@ func WithRowLineage() ScanOption {
func (t Table) Scan(opts ...ScanOption) *Scan {
s := &Scan{
- identifier: t.identifier,
+ identifier: slices.Clone(t.identifier),
Review Comment:
Same class as this Scan path — `Table.Scan()` clones here now, but the
parallel `Transaction.Scan()` at `transaction.go:2177` still does `identifier:
t.tbl.identifier` raw. A `ScanPlanner` that retains the slice past `PlanFiles`
and mutates it would corrupt the owning table's identity through that path.
`slices.Clone` there too (`slices` is already imported in `transaction.go`).
`TestScanPlanningPassesIdentifierCopy` builds the `Scan` by hand, so it
doesn't cover `Transaction.Scan()` — once that test can actually fail (see the
other comment), pointing one variant through `Transaction.Scan()` would close
this.
##########
table/table.go:
##########
@@ -531,7 +533,7 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
return nil, context.Cause(retryCtx)
}
- newMeta, newLoc, err = t.cat.CommitTable(retryCtx,
t.identifier, reqs, updates)
+ newMeta, newLoc, err = t.cat.CommitTable(retryCtx,
slices.Clone(t.identifier), reqs, updates)
Review Comment:
The retry branch here is the one that's still open from last round. I listed
`table.go:480` alongside `Refresh` and `CommitTable` — those two got the clone,
but the `LoadTable` at the top of the retry loop (line 479, just above this)
still hands `t.identifier` straight to the catalog. Same `CatalogIO` boundary,
same fix: `t.cat.LoadTable(retryCtx, slices.Clone(t.identifier))`.
Worth a test too — the `identifierCapturingCatalog` scaffolding extends
naturally to force a retry (first `CommitTable` fails, capture the identifier
on the second `LoadTable`, mutate it, assert `t.Identifier()` is unchanged). If
you go that route, `loadIdentifier` will want to accumulate into a slice since
it's last-write-wins today.
--
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]
