jorgecarleitao commented on pull request #9602: URL: https://github.com/apache/arrow/pull/9602#issuecomment-792275857
Thanks @sundy-li for the explanation. > The partial_sort crate seems also quite small (about 60 lines without tests/imports). What about having the code in Arrow for now, and (try to) limit the amount of unsafety @sundy-li and we can have a next review iteration? I do not follow the argument. Isn't the whole purpose of crates and dependencies to separate concerns and allow code reuse across the ecosystem without dragging dependencies? From what I've seen, the crate: * addresses one problem and one alone (unlike our repo or our workspace) * has CI and CD in place * it is easy to publish in crates.io (via a github action) which makes CD also easy * it is duo license, as it is the standard in Rust * it uses SemVer If I were @sundy-li, I would keep the setup exactly like it is: it enables the implementation to be used by more projects without having to drag arrow and its dependencies with it, while at the same time maintains the generality that the algorithm itself has. I would be behaving exactly how @sundy-li is behaving: engage with potential "customers" like Arrow to identify value, how it can be adopted, and what are the points to improve. From our side, I am not sure I agree with copy-pasting code from a crate to our own crate. IMO we should instead actively engage with our "providers" by contributing upstream. In this particular case, one step is to have MIRI run on CI. The other is to either limit or document the usage of `unsafe`. This way, the Rust community as a whole benefits from these, instead of the benefit be limited to users of the Arrow crate. I created a PR upstream to address one of the steps: https://github.com/sundy-li/partial_sort/pull/1 . Side note: @alamb , what just happened (on which I could trivially PR upstream without any complications, issues, etc.) is a concrete example of what I have been trying to express on the mailing list wrt to the UX that IMO our repo lacks: I can't do that on this repo. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
