alamb commented on a change in pull request #809:
URL: https://github.com/apache/arrow-rs/pull/809#discussion_r717012414
##########
File path: arrow/src/buffer/mod.rs
##########
@@ -23,7 +23,7 @@ pub use immutable::*;
mod mutable;
pub use mutable::*;
mod ops;
-pub(super) use ops::*;
+pub use ops::*;
Review comment:
Would `pub(crate)` work for your usecase?
I somewhat worry about exposing them publicly (and thus they become part of
the required arrow API)?
However, I acknowledge this is a somewhat vague concern
cc @nevi-me @jorgecarleitao @jhorstmann
##########
File path: arrow/src/buffer/mod.rs
##########
@@ -23,7 +23,7 @@ pub use immutable::*;
mod mutable;
pub use mutable::*;
mod ops;
-pub(super) use ops::*;
+pub use ops::*;
Review comment:
>So it seems like there is a bigger discussion here of what is necessary
to make it feasible for the user to write kernels operating on buffers. The
reason I want to make it public, is that it is the minimum change (since the
generic implementation of nullif is tied up in discussion) that enables writing
that within my project, given the problems with the Buffer interface.
Your description makes sense, and to be honest I am fine with merging this
change (though I would like some other perspectives before doing so).
I think something that injects some uncertainty into this discussion is what
the timeframe for integrating some of the changes that are in arrow2 (which
among other things I think basically replaces `Buffer` with an alternate
implementation).
##########
File path: arrow/src/buffer/mod.rs
##########
@@ -23,7 +23,7 @@ pub use immutable::*;
mod mutable;
pub use mutable::*;
mod ops;
-pub(super) use ops::*;
+pub use ops::*;
Review comment:
> Not sure if a big refactoring is worth it if there is a potential
switch to arrow2 in the future. On the other hand a big-bang switch to arrow2
seems a bit scary.
I agree it is a bit scary. Here is an interesting PR in DataFusion that is
close to being fully reviewable that uses arrow2
https://github.com/apache/arrow-datafusion/pull/68 -- it is interesting to note
that most changes are fairly mechanical (there are a few kernels that were
moved, and `StringArray` was renamed `Utf8Array` type stuff , and the
`ArrayBuilders` was replaced with `MutableArray`
Of course, DataFusion doesn't really use the lower level arrow apis (like
Buffer or ArrayData) so I don't really have a sense for how much churn
applications using that lower level API would experience switching to an arrow2
based implementation
--
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]
