You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/05/23 20:31:52 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
paleolimbot opened a new pull request, #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201
The initial motivation for this refactor was to make it possible to go straight from Arrow IPC to ArrowArrayView with zero heap allocations; however, the changes needed for that (basically, give every ArrowArrayView its own offset, length, and null_count) allowed unifying the validation that we do when building arrays and the validation we do when wrapping a foregin ArrowArray in an ArrowArrayView. Along the way, the error messages were improved, too.
TODO: tests passing, but need to clean up and document some of the changes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201#issuecomment-1562135053
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#201](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (02cac45) into [main](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/64944714534d488593ee930d383eb5581fb440fe?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6494471) will **decrease** coverage by `0.44%`.
> The diff coverage is `93.61%`.
```diff
@@ Coverage Diff @@
## main #201 +/- ##
==========================================
- Coverage 88.33% 87.90% -0.44%
==========================================
Files 60 60
Lines 9022 9112 +90
==========================================
+ Hits 7970 8010 +40
- Misses 1052 1102 +50
```
| [Impacted Files](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/nanoarrow.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL25hbm9hcnJvdy9uYW5vYXJyb3cuaA==) | `100.00% <ø> (ø)` | |
| [src/nanoarrow/nanoarrow\_types.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL25hbm9hcnJvdy9uYW5vYXJyb3dfdHlwZXMuaA==) | `92.75% <ø> (ø)` | |
| [src/nanoarrow/array.c](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL25hbm9hcnJvdy9hcnJheS5j) | `93.65% <93.02%> (-2.22%)` | :arrow_down: |
| [r/R/as-array.R](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9SL2FzLWFycmF5LlI=) | `97.01% <100.00%> (+0.06%)` | :arrow_up: |
| [src/nanoarrow/array\_inline.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL25hbm9hcnJvdy9hcnJheV9pbmxpbmUuaA==) | `89.83% <100.00%> (ø)` | |
... and [3 files with indirect coverage changes](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/201/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] lidavidm commented on a diff in pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201#discussion_r1204723524
##########
src/nanoarrow/array.c:
##########
@@ -296,6 +296,16 @@ static ArrowErrorCode ArrowArrayViewInitFromArray(struct ArrowArrayView* array_v
ArrowArrayViewInitFromType(array_view, private_data->storage_type);
array_view->layout = private_data->layout;
array_view->array = array;
+ array_view->length = array->length;
+ array_view->offset = array->offset;
+ array_view->null_count = array->null_count;
Review Comment:
Ah, so is this an additional offset, or is it supposed to replace the array's offset?
##########
src/nanoarrow/array_inline.h:
##########
@@ -654,7 +654,7 @@ static inline void ArrowArrayViewMove(struct ArrowArrayView* src,
static inline int8_t ArrowArrayViewIsNull(struct ArrowArrayView* array_view, int64_t i) {
const uint8_t* validity_buffer = array_view->buffer_views[0].data.as_uint8;
- i += array_view->array->offset;
+ i += array_view->offset;
Review Comment:
From the docstring, shouldn't we _also_ add ArrowArray->offset if the array is set?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201#issuecomment-1562136998
Ah ok - so it's to be used _instead_ of the array offset, or it's "absolute" (which is easier to consume by far!)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot merged pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201#issuecomment-1562134743
Good call - that changed of the course of implementing this. It ended up that `array_view->offset` is identical to `array->offset` (although it can/will be different if the view is representing a slice of the original).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #201: refactor: Unify `ArrowArrayView` and `ArrowArray` validation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #201:
URL: https://github.com/apache/arrow-nanoarrow/pull/201#issuecomment-1561892486
I think otherwise this LGTM, I'd just like to clarify the definition of offset here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org