You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/02/24 19:17:55 UTC
[PR] Double open fix [iceberg-rust]
mbutrovich opened a new pull request, #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Partially addresses #2172.
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
- Builds on #2175. Draft until that merges.
- Load Parquet metadata once via `ArrowReaderMetadata::load_async()`, inspect it in-memory for field IDs, then pass it to a single `ParquetRecordBatchStreamBuilder::new_with_metadata()` call. This eliminates the second `create_parquet_record_batch_stream_builder()` call for migrated tables.
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Existing tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#discussion_r2896023561
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -513,14 +522,51 @@ impl ArrowReader {
.with_preload_page_index(should_load_page_index);
if let Some(hint) = metadata_size_hint {
- parquet_file_reader = parquet_file_reader.with_metadata_size_hint(hint);
+ reader = reader.with_metadata_size_hint(hint);
}
- // Create the record batch stream builder, which wraps the parquet file reader
- let options = arrow_reader_options.unwrap_or_default();
- let record_batch_stream_builder =
- ParquetRecordBatchStreamBuilder::new_with_options(parquet_file_reader, options).await?;
- Ok(record_batch_stream_builder)
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ .await
+ .map_err(|e| {
+ Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
+ })?;
+
+ Ok(Arc::clone(arrow_metadata.metadata()))
+ }
+
+ pub(crate) async fn create_parquet_record_batch_stream_builder(
+ data_file_path: &str,
+ file_io: FileIO,
+ should_load_page_index: bool,
+ arrow_reader_options: ArrowReaderOptions,
+ file_size_in_bytes: u64,
+ parquet_metadata: Arc<ParquetMetaData>,
+ ) -> Result<ParquetRecordBatchStreamBuilder<ArrowFileReader>> {
+ let arrow_reader_metadata =
+ ArrowReaderMetadata::try_new(parquet_metadata, arrow_reader_options).map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ "Failed to create ArrowReaderMetadata from ParquetMetaData",
+ )
+ .with_source(e)
+ })?;
+
+ let parquet_file = file_io.new_input(data_file_path)?;
+ let parquet_reader = parquet_file.reader().await?;
+ let parquet_file_reader = ArrowFileReader::new(
+ FileMetadata {
+ size: file_size_in_bytes,
+ },
+ parquet_reader,
+ )
+ .with_preload_column_index(true)
+ .with_preload_offset_index(true)
Review Comment:
These should now be encapsulated in defaults for `ParquetReadOptions` after #2181, which just kept the values they were before. IIRC Column and Offset Index should be written by default in Parquet in all modern implementations, so preloading makes sense now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#issuecomment-4012822115
It passed all the Iceberg Java tests on Comet.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#discussion_r2896003278
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -490,19 +501,17 @@ impl ArrowReader {
Ok(Box::pin(record_batch_stream) as ArrowRecordBatchStream)
}
- pub(crate) async fn create_parquet_record_batch_stream_builder(
+ /// Loads Parquet metadata from storage, handling file size optimization.
+ pub(crate) async fn load_parquet_metadata(
data_file_path: &str,
- file_io: FileIO,
+ file_io: &FileIO,
should_load_page_index: bool,
- arrow_reader_options: Option<ArrowReaderOptions>,
Review Comment:
Thanks for the review @jdockerty! `ArrowReaderOptions` should be required now instead of an `Option`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#discussion_r2890564559
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -513,14 +522,51 @@ impl ArrowReader {
.with_preload_page_index(should_load_page_index);
if let Some(hint) = metadata_size_hint {
- parquet_file_reader = parquet_file_reader.with_metadata_size_hint(hint);
+ reader = reader.with_metadata_size_hint(hint);
}
- // Create the record batch stream builder, which wraps the parquet file reader
- let options = arrow_reader_options.unwrap_or_default();
- let record_batch_stream_builder =
- ParquetRecordBatchStreamBuilder::new_with_options(parquet_file_reader, options).await?;
- Ok(record_batch_stream_builder)
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ .await
+ .map_err(|e| {
+ Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
+ })?;
+
+ Ok(Arc::clone(arrow_metadata.metadata()))
+ }
+
+ pub(crate) async fn create_parquet_record_batch_stream_builder(
+ data_file_path: &str,
+ file_io: FileIO,
+ should_load_page_index: bool,
+ arrow_reader_options: ArrowReaderOptions,
+ file_size_in_bytes: u64,
+ parquet_metadata: Arc<ParquetMetaData>,
+ ) -> Result<ParquetRecordBatchStreamBuilder<ArrowFileReader>> {
+ let arrow_reader_metadata =
+ ArrowReaderMetadata::try_new(parquet_metadata, arrow_reader_options).map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ "Failed to create ArrowReaderMetadata from ParquetMetaData",
+ )
+ .with_source(e)
+ })?;
+
+ let parquet_file = file_io.new_input(data_file_path)?;
+ let parquet_reader = parquet_file.reader().await?;
+ let parquet_file_reader = ArrowFileReader::new(
+ FileMetadata {
+ size: file_size_in_bytes,
+ },
+ parquet_reader,
+ )
+ .with_preload_column_index(true)
+ .with_preload_offset_index(true)
Review Comment:
I'm not super familiar with the depth of details in Parquet, but are these universal enough to be a sane default?
Can you explain the decision on these being hard-coded to `true`? Where as the `page_index` being pre-loaded is decided by the callee with `should_load_page_index`
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -490,19 +501,17 @@ impl ArrowReader {
Ok(Box::pin(record_batch_stream) as ArrowRecordBatchStream)
}
- pub(crate) async fn create_parquet_record_batch_stream_builder(
+ /// Loads Parquet metadata from storage, handling file size optimization.
+ pub(crate) async fn load_parquet_metadata(
data_file_path: &str,
- file_io: FileIO,
+ file_io: &FileIO,
should_load_page_index: bool,
- arrow_reader_options: Option<ArrowReaderOptions>,
Review Comment:
How come the options are being removed here?
It seems that this forces the `Default::default()` usage below at all times, instead of allowing reader options to be provided, is that intended?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#discussion_r2896116511
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -513,14 +522,51 @@ impl ArrowReader {
.with_preload_page_index(should_load_page_index);
if let Some(hint) = metadata_size_hint {
- parquet_file_reader = parquet_file_reader.with_metadata_size_hint(hint);
+ reader = reader.with_metadata_size_hint(hint);
}
- // Create the record batch stream builder, which wraps the parquet file reader
- let options = arrow_reader_options.unwrap_or_default();
- let record_batch_stream_builder =
- ParquetRecordBatchStreamBuilder::new_with_options(parquet_file_reader, options).await?;
- Ok(record_batch_stream_builder)
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ .await
+ .map_err(|e| {
+ Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
+ })?;
+
+ Ok(Arc::clone(arrow_metadata.metadata()))
+ }
+
+ pub(crate) async fn create_parquet_record_batch_stream_builder(
+ data_file_path: &str,
+ file_io: FileIO,
+ should_load_page_index: bool,
+ arrow_reader_options: ArrowReaderOptions,
+ file_size_in_bytes: u64,
+ parquet_metadata: Arc<ParquetMetaData>,
+ ) -> Result<ParquetRecordBatchStreamBuilder<ArrowFileReader>> {
+ let arrow_reader_metadata =
+ ArrowReaderMetadata::try_new(parquet_metadata, arrow_reader_options).map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ "Failed to create ArrowReaderMetadata from ParquetMetaData",
+ )
+ .with_source(e)
+ })?;
+
+ let parquet_file = file_io.new_input(data_file_path)?;
+ let parquet_reader = parquet_file.reader().await?;
+ let parquet_file_reader = ArrowFileReader::new(
+ FileMetadata {
+ size: file_size_in_bytes,
+ },
+ parquet_reader,
+ )
+ .with_preload_column_index(true)
+ .with_preload_offset_index(true)
Review Comment:
Makes sense, thank you!
You can resolve this comment :bow:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#issuecomment-4012181888
Re-running Iceberg Java tests via Comet at https://github.com/apache/datafusion-comet/pull/3642. I ran this change previously as part of https://github.com/apache/datafusion-comet/pull/3551 which has a lot of the changes discussed in #2172, but those have mostly merged/been refactored as individual PRs, so I'll run just this PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(reader): Avoid second `create_parquet_record_batch_stream_builder()` call for migrated tables [iceberg-rust]
Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #2176:
URL: https://github.com/apache/iceberg-rust/pull/2176#discussion_r2896116511
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -513,14 +522,51 @@ impl ArrowReader {
.with_preload_page_index(should_load_page_index);
if let Some(hint) = metadata_size_hint {
- parquet_file_reader = parquet_file_reader.with_metadata_size_hint(hint);
+ reader = reader.with_metadata_size_hint(hint);
}
- // Create the record batch stream builder, which wraps the parquet file reader
- let options = arrow_reader_options.unwrap_or_default();
- let record_batch_stream_builder =
- ParquetRecordBatchStreamBuilder::new_with_options(parquet_file_reader, options).await?;
- Ok(record_batch_stream_builder)
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ .await
+ .map_err(|e| {
+ Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
+ })?;
+
+ Ok(Arc::clone(arrow_metadata.metadata()))
+ }
+
+ pub(crate) async fn create_parquet_record_batch_stream_builder(
+ data_file_path: &str,
+ file_io: FileIO,
+ should_load_page_index: bool,
+ arrow_reader_options: ArrowReaderOptions,
+ file_size_in_bytes: u64,
+ parquet_metadata: Arc<ParquetMetaData>,
+ ) -> Result<ParquetRecordBatchStreamBuilder<ArrowFileReader>> {
+ let arrow_reader_metadata =
+ ArrowReaderMetadata::try_new(parquet_metadata, arrow_reader_options).map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ "Failed to create ArrowReaderMetadata from ParquetMetaData",
+ )
+ .with_source(e)
+ })?;
+
+ let parquet_file = file_io.new_input(data_file_path)?;
+ let parquet_reader = parquet_file.reader().await?;
+ let parquet_file_reader = ArrowFileReader::new(
+ FileMetadata {
+ size: file_size_in_bytes,
+ },
+ parquet_reader,
+ )
+ .with_preload_column_index(true)
+ .with_preload_offset_index(true)
Review Comment:
Makes sense, thank you!
I'll resolve this :bow:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org