alamb commented on code in PR #7111:
URL: https://github.com/apache/arrow-rs/pull/7111#discussion_r2010826146
##########
parquet/README.md:
##########
@@ -84,7 +84,7 @@ The `parquet` crate provides the following features which may
be enabled in your
- [ ] Row record writer
- [x] Arrow record writer
- [x] Async support
- - [ ] Encrypted files
+ - [x] Encrypted files
Review Comment:
🎉
##########
parquet/src/file/writer.rs:
##########
@@ -699,34 +803,61 @@ impl<'a> SerializedColumnWriter<'a> {
/// `SerializedPageWriter` should not be used after calling `close()`.
pub struct SerializedPageWriter<'a, W: Write> {
sink: &'a mut TrackedWrite<W>,
+ #[cfg(feature = "encryption")]
+ page_encryptor: Option<PageEncryptor>,
+ #[cfg(not(feature = "encryption"))]
+ page_encryptor: Option<Never>,
Review Comment:
Instead of adding a special field, what if you put access to
`page_encryptor` behind a feature flag
```rust
#[cfg(feature = "encryption")]
fn page_encryptor_mut(&mut self) -> Option<&PageEncryptor> {
self.page_encryptor.as_mut()
}
#[cfg(not(feature = "encryption"))]
fn page_encryptor_mut(&mut self) -> Option<&PageEncryptor> {
None
}
```
Perhaps what you could do is then define two `PageEncryptor`s:
1. When `encryption` is enabled, what is currently in this PR
2. When `encryption` is not enabled, then define a `PageEncryptor ` that has
the same interface but function calls that do nothing
##########
parquet/src/file/writer.rs:
##########
@@ -523,12 +595,37 @@ impl<'a, W: Write + Send> SerializedRowGroupWriter<'a, W>
{
) -> Result<C>,
{
self.assert_previous_writer_closed()?;
+
+ #[cfg(feature = "encryption")]
Review Comment:
If at all possible, I would prefer that all the feature gated code is
isolated to functions / structures that have different implementations when the
feature is on/off
Sprinking the `cfg` in this code I think makes it significantly harder to
read and this is some of the most complicated / performance sensitive code of
the parquet crate as it is
##########
parquet/src/util/never.rs:
##########
@@ -0,0 +1,21 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Type that can never be instantiated, to make working with features more
ergonomic.
+// An Option containing this type has zero size.
+#[cfg(not(feature = "encryption"))]
+pub type Never = core::convert::Infallible;
Review Comment:
I left an alternate suggestion above about how to handle the feature flags
--
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
