blackmwk commented on code in PR #3514:
URL: https://github.com/apache/iceberg-rust/pull/3514#discussion_r4523092880
##########
crates/iceberg/src/arrow/reader/mod.rs:
##########
@@ -168,16 +105,99 @@ impl ArrowReaderBuilder {
/// Reads data from Parquet files
#[derive(Clone)]
pub struct ArrowReader {
- batch_size: Option<usize>,
+ options: ArrowReaderOptions,
file_io: FileIO,
delete_file_loader: CachingDeleteFileLoader,
- /// the maximum number of data files that can be fetched at the same time
+ /// Effective concurrency after resolving the host default and normalizing
zero.
concurrency_limit_data_files: usize,
Review Comment:
Do we really need to keep this? I perfer to remove this toally, it could be
stored in `options` totally.
##########
crates/iceberg/src/arrow/reader/options.rs:
##########
@@ -15,70 +15,286 @@
// specific language governing permissions and limitations
// under the License.
-//! Tunables for Parquet file I/O used by `ArrowReader`.
-
+use serde::{Deserialize, Serialize};
use typed_builder::TypedBuilder;
use super::{
DEFAULT_METADATA_SIZE_HINT, DEFAULT_RANGE_COALESCE_BYTES,
DEFAULT_RANGE_FETCH_CONCURRENCY,
};
+/// Configures Arrow reads independently of file planning and local I/O
resources.
+///
+/// Serializable alongside planned file tasks so readers can be constructed
later.
+/// In self-describing map formats such as JSON, field names are part of the
+/// serialized format: missing fields use defaults and unknown fields are
ignored.
+/// Compatibility across different crate versions is not otherwise guaranteed.
+///
+/// ```no_run
+/// # use iceberg::{Result, arrow::{ArrowReaderOptions, ParquetReadOptions},
table::Table};
+/// # async fn read(table: &Table) -> Result<()> {
+/// let options = ArrowReaderOptions::builder()
+/// .with_batch_size(1024)
+/// .with_parquet_options(
+/// ParquetReadOptions::builder()
+/// .with_metadata_size_hint(512 * 1024)
+/// .build(),
+/// )
+/// .build();
+/// let stream = table.scan().build()?.to_arrow(options).await?;
+/// # Ok(())
+/// # }
+/// ```
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, TypedBuilder)]
+#[serde(default)]
+#[builder(field_defaults(default, setter(prefix = "with_")))]
+pub struct ArrowReaderOptions {
+ /// Desired batch size, or the reader default when unset.
+ #[builder(default = None, setter(strip_option))]
+ batch_size: Option<usize>,
+ /// Maximum in-flight data files, or the construction path's default when
unset.
+ #[builder(default = None, setter(strip_option))]
+ data_file_concurrency_limit: Option<usize>,
+ /// Whether to prune row groups using their statistics.
+ #[builder(default = true)]
+ row_group_filtering_enabled: bool,
+ /// Whether to use page indexes for row selection.
+ row_selection_enabled: bool,
+ /// Whether to use bloom filters for row-group pruning.
+ bloom_filter_enabled: bool,
+ /// I/O tuning applied when reading Parquet files.
+ parquet_options: ParquetReadOptions,
+}
+
+impl Default for ArrowReaderOptions {
+ fn default() -> Self {
+ Self::builder().build()
+ }
+}
+
+impl ArrowReaderOptions {
+ /// Supplies a fallback limit without replacing an explicit reader limit.
+ pub(super) fn with_data_file_concurrency_limit_if_unset(mut self, limit:
usize) -> Self {
Review Comment:
This seems unnecessary to me, why not make `data_file_concurrency_limit:
NonZeroUsize` and its default value to `available_parallelism().get().max(1)`
--
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]
