laskoviymishka commented on code in PR #1462:
URL: https://github.com/apache/iceberg-go/pull/1462#discussion_r3577770954
##########
table/table.go:
##########
@@ -866,9 +866,9 @@ func WithLimit(n int64) ScanOption {
}
}
-// WitMaxConcurrency sets the maximum concurrency for table scan and plan
+// WithScanMaxConcurrency sets the maximum concurrency for table scan and plan
// operations. When unset it defaults to runtime.GOMAXPROCS.
-func WitMaxConcurrency(n int) ScanOption {
+func WithScanMaxConcurrency(n int) ScanOption {
Review Comment:
This is the one thing I'd want to settle before we merge, since the name
gets locked in by the deprecation alias — re-changing it later means another
deprecation cycle.
`WithScanMaxConcurrency` is a bit of an odd duck in two ways. Every other
`ScanOption` drops the `Scan` infix (`WithLimit`, `WithRowFilter`,
`WithSnapshotID`, `WithCaseSensitive`), and every other concurrency option
follows `With<Operation>Concurrency` with no `Max` (`WithAddFilesConcurrency`,
`WithOverwriteConcurrency`, `WithCompactionScanConcurrency`). So this name cuts
against both conventions at once.
The clean name is `WithMaxConcurrency`, but `orphan_cleanup.go:118` owns it.
Two ways out: rename that one to `WithCleanupMaxConcurrency` and free up
`WithMaxConcurrency` here (a second breaking rename, but the namespace ends up
right), or go with `WithScanConcurrency` to match the
`With<Operation>Concurrency` family. If `Max` is deliberate — signalling an
upper bound vs a fixed count — I'd say so in the doc. wdyt?
##########
table/table.go:
##########
@@ -878,6 +878,14 @@ func WitMaxConcurrency(n int) ScanOption {
}
}
+// WitMaxConcurrency is a deprecated alias for [WithScanMaxConcurrency], kept
for
+// backwards compatibility with the pre-existing typo'd name.
+//
+// Deprecated: use [WithScanMaxConcurrency].
+func WitMaxConcurrency(n int) ScanOption {
Review Comment:
Neither name has a test — `scan_options_test.go` only covers
`WithSelectedFields` today. A ~10-line table-driven case covering `n>0` (sets
`scan.concurrency`), `n<=0` (returns `noopOption`), and alias/canonical parity
would lock the forwarding down so the alias can't silently drift. wdyt?
##########
table/table.go:
##########
@@ -878,6 +878,14 @@ func WitMaxConcurrency(n int) ScanOption {
}
}
+// WitMaxConcurrency is a deprecated alias for [WithScanMaxConcurrency], kept
for
+// backwards compatibility with the pre-existing typo'd name.
Review Comment:
tiny thing while we're here — the codebase uses "backward compatibility"
without the s (`orphan_cleanup.go:163`, `name_mapping.go:134`). I'd match that.
--
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]
