etseidl commented on PR #9367:
URL: https://github.com/apache/arrow-rs/pull/9367#issuecomment-3863045797
> > 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 like
that. 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>,
> }
> ```
Yes, this is what I was thinking. If we did this, then users could simply
use `set_compression`, and then codec specific options don't creep into the
options. I personally think that's worth a breaking change (and the window is
currently open for those). I'm not sure how big the ripple effect is, though,
if we change the enum.
I wonder if others could opine here before starting down that road. CC
@alamb @jhorstmann @Dandandan @Jefffrey
--
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]
