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/06/07 17:09:03 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
paleolimbot opened a new pull request, #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211
Total proof-of-concept of one way to go about async `stream$get_next()`, as discussed briefly with @krlmlr, @lidavidm, and @nbenn in connection with ADBC in R. When the stream is wrapping a database result set, the `get_next()` call can block for a considerable amount of time...this async version would support a world where it's easier to cancel running queries since we can loop + wait in R land (possibly calling the brand-new "cancel" function coming to ADBC 1.1).
This PR currently will leak memory and doesn't communicate any error information beyond "result code".
Reprex:
``` r
library(nanoarrow)
stream <- basic_array_stream(list(1:5))
# ...more usefully, a promise or future whose value is set from the callback
result <- NULL
nanoarrow:::nanoarrow_array_stream_get_next_async(stream, function(code, array) {
message("done!")
result <<- array
})
Sys.sleep(0.5)
nanoarrow:::run_callbacks()
#> done!
convert_array(result)
#> [1] 1 2 3 4 5
```
<sup>Created on 2023-06-07 with [reprex v2.0.2](https://reprex.tidyverse.org)</sup>
--
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 closed pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot closed pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
URL: https://github.com/apache/arrow-nanoarrow/pull/211
--
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] krlmlr commented on pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "krlmlr (via GitHub)" <gi...@apache.org>.
krlmlr commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1581221138
🤩 🤩 🤩
Give me some time to dig deeper into it. I might add a promises-based wrapper around that.
--
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] krlmlr commented on pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "krlmlr (via GitHub)" <gi...@apache.org>.
krlmlr commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1582555274
I was wondering if the thread should be a property of the connection object. That way we don't need to create and destroy threads, only wake them for each operation (limitless producer-consumer design?).
--
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 #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1581285324
Just thinking out loud, but maybe a better home for this would be adbcdrivermanager (where a dependency on later/promises would be more than worth it to support async).
--
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 #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1582769804
That's a great idea. This functionality is much better suited to adbcdrivermanager anyway and keeping all the async stuff in one place will be very helpful.
--
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 #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1666932476
Sorry for the delay...I was taking some time away from the keyboard!
I think the best place for this right now is the ADBC R bindings since there will also be other async-related code and it is probably best to keep that in the same place. I'm happy to take direction on that...you have spent more time considering async database access than I have and the purpose of this change would be for the express purpose of enabling that in ADBC/R.
--
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] krlmlr commented on pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "krlmlr (via GitHub)" <gi...@apache.org>.
krlmlr commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1581462263
Yeah, if we can expose the relevant C interface here, we can support a higher-level interface in adbcdrivermanager.
Sequence diagrams FTW!
--
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 #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1581226558
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#211](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3f6214c) into [main](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/47a4ef4113d6539db14713f8696b01012e0022df?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (47a4ef4) will **increase** coverage by `0.06%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## main #211 +/- ##
==========================================
+ Coverage 87.87% 87.94% +0.06%
==========================================
Files 60 60
Lines 9249 9301 +52
==========================================
+ Hits 8128 8180 +52
Misses 1121 1121
```
| [Impacted Files](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [r/src/init.c](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9zcmMvaW5pdC5j) | `100.00% <ø> (ø)` | |
| [r/src/util.h](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9zcmMvdXRpbC5o) | `100.00% <ø> (ø)` | |
| [r/R/array-stream.R](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9SL2FycmF5LXN0cmVhbS5S) | `96.10% <100.00%> (+0.32%)` | :arrow_up: |
| [r/R/util.R](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9SL3V0aWwuUg==) | `90.24% <100.00%> (+0.24%)` | :arrow_up: |
| [r/src/array\_stream.c](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9zcmMvYXJyYXlfc3RyZWFtLmM=) | `88.67% <100.00%> (+0.32%)` | :arrow_up: |
| [r/src/nanoarrow\_cpp.cc](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9zcmMvbmFub2Fycm93X2NwcC5jYw==) | `97.72% <100.00%> (+1.89%)` | :arrow_up: |
| [r/src/util.c](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9zcmMvdXRpbC5j) | `100.00% <100.00%> (ø)` | |
: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] krlmlr commented on pull request #211: feat(r): Add callback-based async option for calling `ArrowArrayStream::get_next()`
Posted by "krlmlr (via GitHub)" <gi...@apache.org>.
krlmlr commented on PR #211:
URL: https://github.com/apache/arrow-nanoarrow/pull/211#issuecomment-1657116238
Do you plan to further enhance this?
--
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