alexanderbianchi commented on code in PR #3514:
URL: https://github.com/apache/iceberg-rust/pull/3514#discussion_r4407276242
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -259,13 +242,6 @@ impl<'a> TableScanBuilder<'a> {
pub fn with_concurrency_limit(mut self, limit: usize) -> Self {
Review Comment:
It’s already Option<usize>, so we could define:
- Some(n) → use the supplied reader limit.
- None through to_arrow() → inherit the scan’s concurrency limit.
- None through direct reader construction → use execution-host parallelism.
to_arrow() would resolve the fallback before passing options into the reader
builder.
That preserves the old scan-level bound without tracking whether it was
explicitly set. The tradeoff is that None becomes context-dependent: through a
scan it inherits the scan’s limit, including its automatically chosen default;
directly through `ArrowReaderBuilder` it uses the reader host’s default. What
do you think? IMO if you are using the `ArrowReaderBuilder` you have signed up
for managing this, and as long as it's documented it makes sense.
We can audit callers in the repository, but we can’t prove that no
downstream user relies on the old bound. This PR deliberately separates
planning concurrency from reader concurrency, alongside the breaking
to_arrow(options) change. To preserve the previous behavior, callers should set
the limit on both the scan and ArrowReaderOptions. Inheriting it implicitly
would retain the coupling we’re removing.
I'm open to including in the migration here that concurrency will have to be
explicitly set, but keeping the default seems sane to me too. If we want to
eventually isolate the options, now is the best time to do it so users can
update the same time they move to `Default::options()`.
--
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]
