Use Arc<FileDecryptionProperties> to reduce size of ParquetMetadata and avoid copying when encryption is enabled
#8470
Conversation
…etMetadata and avoid copying
alamb
force-pushed
the
alamb/file_encryption_properties
branch
from
5355828 to
2666244
Compare
| Ok(Self { | ||
| footer_decryptor: Arc::new(footer_decryptor), | ||
| decryption_properties: decryption_properties.clone(), | ||
| decryption_properties: Arc::clone(decryption_properties), |
There was a problem hiding this comment.
This is one example of a clone (deep copy) that is removed due to this change
| 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 |
There was a problem hiding this 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:
| file_encryption_properties.validate_encrypted_column_names(schema_descriptor)?; | ||
|
|
||
| Ok(Some(Arc::new(FileEncryptor::new( | ||
| file_encryption_properties.clone(), |
There was a problem hiding this comment.
Here is another clone that is removed by this PR
| pub fn with_file_decryption_properties( | ||
| self, | ||
| file_decryption_properties: FileDecryptionProperties, | ||
| file_decryption_properties: Arc<FileDecryptionProperties>, |
There was a problem hiding this comment.
this and the change to file_decryption_properties is an API change
|
|
||
| /// Finalize the builder and return created [`FileDecryptionProperties`] | ||
| pub fn build(self) -> Result<FileDecryptionProperties> { | ||
| pub fn build(self) -> Result<Arc<FileDecryptionProperties>> { |
There was a problem hiding this 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
alamb
changed the title
Arc<FileDecryptionProperties> to reduce size of ParquetMetadata and avoid copyingArc<FileDecryptionProperties> to reduce size of ParquetMetadata and avoid copying when encryption is enabled
There was a problem hiding this comment.
This looks good to me thanks Andrew, I think it's fine to change the API to use an Arc (and similarly for the encryption properties), especially as this feature is still experimental.
There are some conflicts with main now though. Let me know if you want me or @rok to help with this and clean up our code!
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.
Rationale for this change
While working on other PRs I noticed a lot of copying and that
ParquetMetaDatais significantly larger when theencryptionfeature is enabled This is unnecessary and could be avoidedby using
Arc<FileDecryptionProperties>instead ofFileDecryptionPropertiesdirectly.What changes are included in this PR?
FileDecryptionPropertiesintoArc<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 thanFileDecryptionProperties