You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/09/26 14:18:35 UTC
[PR] Refactor: Use `Arc` to reduce size of ParquetMetadata and avoid copying [arrow-rs]
alamb opened a new pull request, #8470:
URL: https://github.com/apache/arrow-rs/pull/8470
# 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.
- Closes https://github.com/apache/arrow-rs/issues/8469
# Rationale for this change
While working on other PRs I noticed a lot of copying and that `ParquetMetaData` is significantly larger when the `encryption` feature is enabled This is unnecessary and could be avoided
by using `Arc<FileDecryptionProperties>` instead of `FileDecryptionProperties` directly.
# What changes are included in this PR?
1. Change most uses of `FileDecryptionProperties` into `Arc<FileDecryptionProperties>`
If this is an acceptable API change, I will make a follow on PR giving the same treatment to FileEncryptionProperties
(turn it into `Arc<FileEncryptionProperties>`)
# Are these changes tested?
Yes by CI
# Are there any user-facing changes?
Yes, some of the APIs now take `Arc<FileDecryptionProperties>` rather than `FileDecryptionProperties`
--
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] Use `Arc` to reduce size of ParquetMetadata and avoid copying when `encryption` is enabled [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470
--
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] Use `Arc` to reduce size of ParquetMetadata and avoid copying when `encryption` is enabled [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470#issuecomment-3406823288
After trying to integrate this into DataFusion I found a few rough edges, so I filed a small follow on PR:
- https://github.com/apache/arrow-rs/pull/8626
--
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] Use `Arc` to reduce size of ParquetMetadata and avoid copying when `encryption` is enabled [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470#issuecomment-3403470977
Merging this one in to get it into arrow 57 as well as avoid more merge conflicts@
--
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] Refactor: Use `Arc` to reduce size of ParquetMetadata and avoid copying [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470#discussion_r2382621511
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -565,7 +565,7 @@ impl FileDecryptor {
Ok(Self {
footer_decryptor: Arc::new(footer_decryptor),
- decryption_properties: decryption_properties.clone(),
+ decryption_properties: Arc::clone(decryption_properties),
Review Comment:
This is one example of a `clone` (deep copy) that is removed due to this change
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -1974,7 +1974,8 @@ mod tests {
#[cfg(not(feature = "encryption"))]
let base_expected_size = 2312;
#[cfg(feature = "encryption")]
- let base_expected_size = 2648;
+ // Not as accurate as it should be: https://github.com/apache/arrow-rs/issues/8472
Review Comment:
This PR does actually reduce heap size needed when the `encryption` feature is enabled, but there are no FileDecryptionProperties -- instead of having space inline for the entire `FileDecryptionProperties` now there is only an `Arc` (a pointer + refcount).
However, while reviewing this, I found the heap size accounting is somewhat inaccurate for encryption and filed a second ticket:
- https://github.com/apache/arrow-rs/issues/8472
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -508,7 +508,7 @@ impl ArrowReaderOptions {
#[cfg(feature = "encryption")]
pub fn with_file_decryption_properties(
self,
- file_decryption_properties: FileDecryptionProperties,
+ file_decryption_properties: Arc<FileDecryptionProperties>,
Review Comment:
this and the change to `file_decryption_properties` is an API change
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -438,16 +438,16 @@ impl DecryptionPropertiesBuilder {
}
/// Finalize the builder and return created [`FileDecryptionProperties`]
- pub fn build(self) -> Result<FileDecryptionProperties> {
+ pub fn build(self) -> Result<Arc<FileDecryptionProperties>> {
Review Comment:
This is also an API change. It is somewhat strange to return an `Arc` from a builder but doing so eases the impact of this change on downstream users -- the output of the builder can be used by the other APIs directly
##########
parquet/src/file/writer.rs:
##########
@@ -215,12 +215,12 @@ impl<W: Write + Send> SerializedFileWriter<W> {
properties: &WriterPropertiesPtr,
schema_descriptor: &SchemaDescriptor,
) -> Result<Option<Arc<FileEncryptor>>> {
- if let Some(file_encryption_properties) = &properties.file_encryption_properties {
+ if let Some(file_encryption_properties) = properties.file_encryption_properties() {
file_encryption_properties.validate_encrypted_column_names(schema_descriptor)?;
- Ok(Some(Arc::new(FileEncryptor::new(
- file_encryption_properties.clone(),
Review Comment:
Here is another clone that is removed by 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] Use `Arc` to reduce size of ParquetMetadata and avoid copying when `encryption` is enabled [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470#issuecomment-3393299690
No problems, -- I will get it ship shape!
--
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] Use `Arc` to reduce size of ParquetMetadata and avoid copying when `encryption` is enabled [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8470:
URL: https://github.com/apache/arrow-rs/pull/8470#issuecomment-3392575335
> There are some conflicts with main now though.
That's likely my doing. Likewise, let me know if you want help getting this ready to ship.
--
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