adamreeve commented on PR #7111:
URL: https://github.com/apache/arrow-rs/pull/7111#issuecomment-2753181854

   I've tried to reduce the use of `#[cfg(feature = "encryption")]` within 
function bodies 
(https://github.com/apache/arrow-rs/pull/7111/commits/78ac7b25504ea0d23d1d612f0cb52e823c012bd5),
 but have been pretty consistently hampered by borrow checker issues. The 
borrow checker has no insight into which fields a function accesses and assumes 
all of self is borrowed by an `&self` function, so it's not possible to extract 
logic out to functions when there's a mutable reference to any field of `self` 
present.
   
   For example, in `arrow_writer/mod.rs`, we currently have this code:
   
   
https://github.com/apache/arrow-rs/blob/78ac7b25504ea0d23d1d612f0cb52e823c012bd5/parquet/src/arrow/arrow_writer/mod.rs#L266-L288
   
   The contents of the `x` branch of the match statement can't be extracted 
into a method, because a mutable reference to `self.in_progress` is live. This 
is the same reason why getting `row_group_index` has to come before the 
reference to `self.in_progress` is taken.
   
   I ran into a similar problem in `file/metadata/writer.rs` here: 
https://github.com/apache/arrow-rs/blob/78ac7b25504ea0d23d1d612f0cb52e823c012bd5/parquet/src/file/metadata/writer.rs#L73
   
   I wanted to extract a `write_offset_index` method with different 
implementations depending on whether encryption is enabled, but this isn't 
possible due to there being a mutable reference to `self.row_groups`. I could 
instead have different implementations of the whole `write_offset_indexes` 
function, but that would lead to quite a bit of code duplication.
   
   I think the current state is a bit better than it was before, but any 
thoughts on how to work around this and improve the code readability further 
would be appreciated! 
   
   `ThriftMetadataWriter::finish` is probably the ugliest part of the diff at 
the moment, mostly because of a workaround we have to return unencrypted row 
group metadata so that the statistics are usable by consumers, and we'd also 
appreciate any thoughts on that 
(https://github.com/apache/arrow-rs/pull/7111/files#r2009421835).


-- 
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-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to