You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2025/04/01 16:27:05 UTC
[PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
etseidl opened a new pull request, #7369:
URL: https://github.com/apache/arrow-rs/pull/7369
# 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.
-->
Might fix #6476.
# Rationale for this change
`ArrowReaderMetadata::load_async` sometimes had to do multiple passes to fully load Parquet metadata when page indexes were requested. This is because `AsyncFileReader::get_metadata` function has no way of knowing if page indexes are desired. Recent API changes have allowed for passing this information to `AsyncFileReader`, so the extra page index logic in `load_async` should no longer be necessary.
This version will still do multiple fetches since no prefetch hint is passed to the metadata reader. A follow on PR could add this hint to `ArrowReaderOptions`, but that would be a breaking change.
# What changes are included in this PR?
Convert `AsyncFileReader::get_metadata` to use the new `ParquetMetaDataReader::load_via_suffix_and_finish` API to reduce code duplication and add a `MetadataSuffixFetch` implementation to allow its use in `ArrowReaderMetadata::load_async`.
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023781763
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -192,26 +188,7 @@ impl ArrowReaderMetadata {
input: &mut T,
options: ArrowReaderOptions,
) -> Result<Self> {
- // TODO: this is all rather awkward. It would be nice if AsyncFileReader::get_metadata
- // took an argument to fetch the page indexes.
- let mut metadata = input.get_metadata(Some(&options)).await?;
-
- if options.page_index
- && metadata.column_index().is_none()
- && metadata.offset_index().is_none()
- {
- let m = Arc::try_unwrap(metadata).unwrap_or_else(|e| e.as_ref().clone());
- let mut reader = ParquetMetaDataReader::new_with_metadata(m).with_page_indexes(true);
-
- #[cfg(feature = "encryption")]
- {
- reader =
- reader.with_decryption_properties(options.file_decryption_properties.as_ref());
- }
-
- reader.load_page_index(input).await?;
- metadata = Arc::new(reader.finish()?)
- }
+ let metadata = input.get_metadata(Some(&options)).await?;
Review Comment:
Ah, yes it should. Thank you 🙏
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2027062190
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
Review Comment:
Thank clippy 😅
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023683557
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
Filed #7372
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2027033491
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
Review Comment:
TIL: is_some_and 👍
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
I don't remember the discussion about suffix requests, but I went over this logic carefully and it makes a lot of sense to me
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023589215
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
Agreed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#issuecomment-2779826199
Thanks again @etseidl @adamreeve and @kylebarron
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023556526
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
Is this a bug in the existing functionality, or introduced in 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023566643
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
Yes, I recall. But the code that's replaced already did seeks from the end of the file. I assume it's ok here because it's not really an object store, just async access to a file in most instances.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023561631
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
I think there was some pushback prior to defaulting to using suffix requests for loading metadata. (Although this looks like it's the impl on `AsyncRead` and `AsyncSeek`, and not the `ParquetObjectReader`, so it may be fine)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#issuecomment-2769935660
cc @adamreeve @kylebarron
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023561631
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
I think there was some pushback prior to defaulting to using suffix requests for loading metadata.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2027204396
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -147,31 +158,16 @@ impl<T: AsyncRead + AsyncSeek + Unpin + Send> AsyncFileReader for T {
&'a mut self,
options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, Result<Arc<ParquetMetaData>>> {
- const FOOTER_SIZE_I64: i64 = FOOTER_SIZE as i64;
async move {
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64)).await?;
-
- let mut buf = [0_u8; FOOTER_SIZE];
- self.read_exact(&mut buf).await?;
-
- let footer = ParquetMetaDataReader::decode_footer_tail(&buf)?;
- let metadata_len = footer.metadata_length();
-
- self.seek(SeekFrom::End(-FOOTER_SIZE_I64 - metadata_len as i64))
- .await?;
-
- let mut buf = Vec::with_capacity(metadata_len);
- self.take(metadata_len as _).read_to_end(&mut buf).await?;
-
- let metadata_reader = ParquetMetaDataReader::new();
+ let metadata_reader = ParquetMetaDataReader::new()
+ .with_page_indexes(options.is_some_and(|o| o.page_index));
#[cfg(feature = "encryption")]
let metadata_reader = metadata_reader.with_decryption_properties(
options.and_then(|o| o.file_decryption_properties.as_ref()),
);
- let parquet_metadata = metadata_reader.decode_footer_metadata(&buf, &footer)?;
-
+ let parquet_metadata = metadata_reader.load_via_suffix_and_finish(self).await?;
Review Comment:
I was referring to @Xuanwo 's comment here https://github.com/apache/arrow-rs/pull/6157#discussion_r1724380135. But I think that was specifically referring to Azure object storage, and here since we're working with the generic tokio traits, it should be fine.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023603238
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
Perhaps we should add a test for this ASAP before the imminent release? cc @alamb
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023563754
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
I haven't yet tested that...I don't know if suffix reading paired with encryption has been tested before now. Certainly this PR triggered it due to a change in how the async reader gets its bytes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023730330
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
Thank you!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#issuecomment-2775894762
I also found myself with the urge to improve the documentation:
- https://github.com/apache/arrow-rs/pull/7380
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#issuecomment-2779826076
Let's do it!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023646219
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -710,7 +710,11 @@ impl ParquetMetaDataReader {
));
}
- Ok((self.decode_footer_metadata(&meta, &footer)?, None))
+ // need to slice off the footer or decryption fails
+ Ok((
+ self.decode_footer_metadata(&meta.slice(0..length), &footer)?,
Review Comment:
I just did a quick ugly test and verified this is an issue with not slicing of the footer. The code in the `else` block does perform this slice. I can split this out into a separate PR so it can go in before 55.0.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Clean up `ArrowReaderMetadata::load_async` [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7369:
URL: https://github.com/apache/arrow-rs/pull/7369#discussion_r2023774088
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -192,26 +188,7 @@ impl ArrowReaderMetadata {
input: &mut T,
options: ArrowReaderOptions,
) -> Result<Self> {
- // TODO: this is all rather awkward. It would be nice if AsyncFileReader::get_metadata
- // took an argument to fetch the page indexes.
- let mut metadata = input.get_metadata(Some(&options)).await?;
-
- if options.page_index
- && metadata.column_index().is_none()
- && metadata.offset_index().is_none()
- {
- let m = Arc::try_unwrap(metadata).unwrap_or_else(|e| e.as_ref().clone());
- let mut reader = ParquetMetaDataReader::new_with_metadata(m).with_page_indexes(true);
-
- #[cfg(feature = "encryption")]
- {
- reader =
- reader.with_decryption_properties(options.file_decryption_properties.as_ref());
- }
-
- reader.load_page_index(input).await?;
- metadata = Arc::new(reader.finish()?)
- }
+ let metadata = input.get_metadata(Some(&options)).await?;
Review Comment:
Should the note in the doc comment above be removed?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org