alexanderbianchi commented on code in PR #3514: URL: https://github.com/apache/iceberg-rust/pull/3514#discussion_r4378681836
########## crates/iceberg/src/arrow/reader/parquet_options.rs: ########## @@ -0,0 +1,84 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Tunables for Parquet file I/O used by `ArrowReader`. + +use typed_builder::TypedBuilder; + +use super::{ + DEFAULT_METADATA_SIZE_HINT, DEFAULT_RANGE_COALESCE_BYTES, DEFAULT_RANGE_FETCH_CONCURRENCY, +}; + +/// Options for tuning Parquet file I/O. +#[derive(Clone, Copy, Debug, TypedBuilder)] +#[builder(field_defaults(setter(prefix = "with_")))] +pub(crate) struct ParquetReadOptions { Review Comment: I see the benefit of making `ParquetReadOptions` public so callers can configure it directly. Storing options structs in `ArrowReaderBuilder` also simplifies the implementation by removing the duplicated individual fields. Two things I’d like to preserve: - Both `scan.to_arrow(options)` and the explicit `plan_files()` + reader path should support the same configuration. - Format-specific types should stay out of the common read signatures, so supporting another format doesn’t require changing `to_arrow()`. With the proposed structure: ```rust pub struct ArrowReaderBuilder { options: ArrowReaderOptions, parquet_options: ParquetReadOptions, // FileIO, runtime } ``` The explicit path can configure both, but `to_arrow(options)` still uses default Parquet settings unless we extend its signature to accept them separately. Could we make `ParquetReadOptions` public but nest it inside `ArrowReaderOptions`? ```rust pub struct ArrowReaderOptions { batch_size: Option<usize>, data_file_concurrency_limit: Option<usize>, // Other reader settings parquet_options: ParquetReadOptions, } pub struct ArrowReaderBuilder { options: ArrowReaderOptions, // FileIO, runtime } ``` That gives both paths one complete configuration: ```rust let options = ArrowReaderOptions::builder() .with_batch_size(Some(1024)) .with_parquet_options( ParquetReadOptions::builder() .with_metadata_size_hint(Some(512 * 1024)) .build(), ) .build(); // Convenience path: let stream = scan.to_arrow(options).await?; // Or explicit reader construction: let reader = ArrowReaderBuilder::new(file_io, runtime) .with_options(options) .build(); ``` We could still expose `ArrowReaderBuilder::with_parquet_options()` to update the nested configuration. Whether to deprecate the individual setters can be a separate decision; this structure works either way. -- 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]
