amirshukayev commented on PR #9367:
URL: https://github.com/apache/arrow-rs/pull/9367#issuecomment-3862904880
> Thanks @amirshukayev, this looks interesting. Since the compression codec
itself is already on `ColumnProperties` rather than `WriterProperties`, I can't
help wondering if the override should be as well. I can see only wanting to
override a single column and leave the others with the default.
Great point! I've made the update to allow it to be set on
`ColumnProperties`.
> I also wonder if we should instead modify `basic::Compression` enum and
add the override to the `ZSTD` variant. That would be more of a breaking
change, but might be more natural 🤷
I think this makes sense, this the approach I started with. Whats your take
here? would it be worth breaking things? I'd be happy to implement it as such,
although my approach. This is the approach I used before, which I assume is
what you mean?
```rust
// Before:
pub struct ZstdLevel(i32);
// After (vendored):
pub struct ZstdLevel {
level: i32,
window_log_override: Option<u32>,
}
impl ZstdLevel {
pub fn try_new(level: i32) -> Result<Self> {
Self::is_valid_level(level).map(|_| Self { level,
window_log_override: None })
}
pub fn with_window_log_override(mut self, window_log: u32) -> Self {
self.window_log_override = Some(window_log);
self
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
