You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/28 05:02:13 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
jorgecarleitao opened a new pull request #8543:
URL: https://github.com/apache/arrow/pull/8543
Some notes:
* Each commit is a different array
* I named the files `array_[type]` to distinguish from other modules that do not contain arrays.
* `null` and `Union` were not renamed
* Some functions were moved around / incorporated in `impl` since they were used in multiple places.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-718789440
Moving the arrays to separate files is nice. I see there are some functionality changes in the PR too. Were these necessary as part of the refactoring or is this separate?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-718851474
Thanks. I just wanted to understand which parts needed review. I don't think there is a need to split into separate PRs.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-718835276
The reason is that there was a private function (`slice_data`) in `array.rs` that was used on two different array types. I moved that function to be part of the `ArrayData`, `ArrayData::slice`, so that it can be used in multiple places (and is really a function that literally slices `ArrayData`). This is change 1.
Since I made `slice` `pub`, I also added a unit test to it (change 2). I also noticed that there was some code duplication between that function (previously in `array.rs`, now in `data.rs`), and thus pulled that duplicated code to a separate function (`count_nulls`) for DRY (change 3). Theses changes implied that the `pub(crate)` data from `ArrayData` was no longer needed to be `pub` at all, and thus I just privatized them (this is not a public change, so all backward compatible), change 4. All these changes are done in the first commit only.
Neither the DRY, unit-testing nor privatization was necessary as part of this PR. I can revert them if you think we should do this cleanup in a separate PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-717701479
https://issues.apache.org/jira/browse/ARROW-9361
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-723435331
I also agree this PR is likely to cause a bunch of conflicts, so the less time it is left outstanding the better.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me closed pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #8543:
URL: https://github.com/apache/arrow/pull/8543
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8543: ARROW-9361 [Rust] Move array types into their own modules
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8543:
URL: https://github.com/apache/arrow/pull/8543#issuecomment-723480908
@alamb and @nevi-me , thanks a lot for taking the time to review this.
I have rebased this against master. I am fine with either order, rebasing this one is tedious but straightforward work as all changes are continuous blocks of code.
----------------------------------------------------------------
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:
users@infra.apache.org