You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "masonh22 (via GitHub)" <gi...@apache.org> on 2024/11/03 19:56:19 UTC
[PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
masonh22 opened a new pull request, #6676:
URL: https://github.com/apache/arrow-rs/pull/6676
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #6559.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
This improves performance when reading from filesystems with high latency and/or low bandwidth.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
This adds an option to `ParquetRecordBatchStream` to load the next row group while decoding the current one. This adds a new state called `Prefetch` to the stream state. In this state, a future for the next row group is polled before returning data from the current `ParquetRecordBatchReader`.
# Are there any user-facing changes?
There is a new option for `ParquetRecordBatchStreamBuilder` called `prefetch` that is set by a method called `with_prefetch`.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2549822340
I apologize @masonh22 for the delay
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "masonh22 (via GitHub)" <gi...@apache.org>.
masonh22 commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2552319377
I imagine that a part of the reason why this PR has stalled is that the design isn't great. It adds a lot of complexity to a part of the code that should be kept simple. It is, however, a very convenient interface for solving the specific problem I was running into.
@Xuanwo that sounds like a much better design IMO, but I think it will be tricky to work that into ParquetRecordBatchStream. Maybe this should be broken out into something new.
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2550193737
Hi, I like this PR. I'm trying to find a way to enable splitting IO-bound and CPU-bound jobs. For a query engine like Databend, we run IO tasks on an async runtime and CPU-bound jobs on dedicated threads. We schedule them using our pipeline system.
Adding `fetch` seems like a good starting point for me. Is it possible to take it a step further by allowing users to call `fetch().await` and then perform `next()` in a separate blocking manner? The challenge might lie in designing an API that is sufficiently well-structured.
cc @alamb, I'm willing to start a new issue if you think this PR is not the right place.
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#discussion_r1827096328
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
Review Comment:
What is the rationale for doing 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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on code in PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#discussion_r1896931818
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
+ match f.poll_unpin(&mut noop_cx) {
Review Comment:
Yeah. I think adding that API is fine as a way to give some more control to the user. But I would love to find a way to really implement IO prefetching. Maybe one day I'll find the time to really sit down and work something out 😂
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #6676: Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding
URL: https://github.com/apache/arrow-rs/pull/6676
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2554397013
> FWIW I did have some ideas about exposing lower level APIs in #5522 that would allow finer grained control of decode but I've not had time, and am unlikely to have time, to write up a concrete proposal.
>
> I agree that we should provide a way to concurrently fetch IO and do parallel decode, it is unclear that this can be made to work in the RecordBatchStream interface, or if it would need a new API as proposed in #5522
Hi @tustvold,
Thank you for the information—it's really helpful! I'll take a look and try out some of the ideas.
> Maybe this should be broken out into something new.
Agreed. I will continue the discussion at #5522 or #1605 instead.
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2553575350
> Sorry that this PR is languishing -- I think it is tricky enough logic that it needs a good thorough review and (and the other maintainers haven't been able to find the time 😢 )
>
> @Dandandan or @thinkharderdev is there any chance someone from one of your teams has the bandwidth to review this PR? I suspect it will likely be an improvement but needs some careful review
I can try and find some time to review this week. Have been thinking about something along these lines as well so happy to see someone is working on it :)
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on code in PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#discussion_r1894947151
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
Review Comment:
The concern I have here is that this effectively decouples polling the underlying IO operations from the reactor. We end up polling the future more or less continuously (assuming the consumer is just polling the stream in a loop during decoding which is likely the case).
A different way to handle this would be to have a `Prefetcher` which can spawn the prefetch op in the background and let the runtime poll it. You can just "assume tokio" in which case the prefetcher is just `tokio::spawn` or try and abstract over async runtime behind a trait
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
+ match f.poll_unpin(&mut noop_cx) {
Review Comment:
When I have tried to think about this problem at various points I get hung up at this part. Theoretically polling the next reader here should just be driving IO (and hence be cheap). But reading the row group includes both IO and evaluating row filters which can involve non-trivial compute and memory since we evaluate the row filter over the entire row group in one shot.
To really implement pre-fetch I think we need to have finer-grained control over the pipelining of IO and cpu operations. E.g. prefetch should only do the next IO operation
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2555730747
> > Sorry that this PR is languishing -- I think it is tricky enough logic that it needs a good thorough review and (and the other maintainers haven't been able to find the time 😢 )
> > @Dandandan or @thinkharderdev is there any chance someone from one of your teams has the bandwidth to review this PR? I suspect it will likely be an improvement but needs some careful review
>
> I can try and find some time to review this week. Have been thinking about something along these lines as well so happy to see someone is working on it :)
Thank you very much @thinkharderdev
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#discussion_r1896783680
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
+ match f.poll_unpin(&mut noop_cx) {
Review Comment:
I think this is aligned with @tustvold 's concerns as well.
What do you think about this PR from @Xuanwo ?
- https://github.com/apache/arrow-rs/pull/6907
It doesn't solve the "poll may do significant CPU" issue you highlight, but I don't think it makes it worse either
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2479968077
This is still on my radar to review, I just haven't had a chance
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "masonh22 (via GitHub)" <gi...@apache.org>.
masonh22 commented on code in PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#discussion_r1828530119
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -660,36 +705,89 @@ where
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
loop {
match &mut self.state {
- StreamState::Decoding(batch_reader) => match batch_reader.next() {
- Some(Ok(batch)) => {
- return Poll::Ready(Some(Ok(batch)));
+ StreamState::Decoding(batch_reader) => {
+ let res: Self::Item = match batch_reader.next() {
+ Some(Ok(batch)) => Ok(batch),
+ Some(Err(e)) => {
+ self.state = StreamState::Error;
+ return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+ }
+ None => {
+ self.state = StreamState::Init;
+ continue;
+ }
+ };
+
+ if !self.prefetch_row_groups
+ || self.row_groups.is_empty()
+ || self.next_reader.is_some()
+ {
+ return Poll::Ready(Some(res));
}
- Some(Err(e)) => {
- self.state = StreamState::Error;
- return Poll::Ready(Some(Err(ParquetError::ArrowError(e.to_string()))));
+
+ let old_state = std::mem::replace(&mut self.state, StreamState::Init);
+
+ let row_group_idx = self.row_groups.pop_front().unwrap(); // already checked that row_groups is not empty
+
+ let fut = self.read_row_group(row_group_idx);
+
+ if let StreamState::Decoding(batch_reader) = old_state {
+ self.state = StreamState::Prefetch(batch_reader, fut);
+ return Poll::Ready(Some(res));
+ } else {
+ unreachable!()
}
- None => self.state = StreamState::Init,
- },
+ }
+ StreamState::Prefetch(batch_reader, f) => {
+ let mut noop_cx = Context::from_waker(futures::task::noop_waker_ref());
Review Comment:
I wanted to avoid any potential overhead from using the real context when polling the future here. Since we're always returning Poll::Ready out of this state (or transitioning to another state), we don't need to rely on the real context to wake the main stream future.
I'm not an expert at async rust code though so if it would make more sense to do something else here I'm happy to make that change.
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2549821973
Sorry that this PR is languishing -- I think it is tricky enough logic that it needs a good thorough review and (and the other maintainers haven't been able to find the time 😢 )
@Dandandan or @thinkharderdev is there any chance someone from one of your teams has the bandwidth to review this PR? I suspect it will likely be an improvement but needs some careful review
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2556278331
Hi, after building https://github.com/apache/arrow-rs/issues/5522, I realized it can also be applied to the use case we were aiming for here. Users can wrap their own `Stream<Item=Result<RecordBatch>>` based on the API we provide.
--
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
Re: [PR] Add a 'prefetch' option to `ParquetRecordBatchStream` to load the next row group while decoding [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6676:
URL: https://github.com/apache/arrow-rs/pull/6676#issuecomment-2551016464
FWIW I did have some ideas about exposing lower level APIs in https://github.com/apache/arrow-rs/issues/5522 that would allow finer grained control of decode but I've not had time, and am unlikely to have time, to write up a concrete proposal.
I agree that we should provide a way to concurrently fetch IO and do parallel decode, it is unclear that this can be made to work in the RecordBatchStream interface, or if it would need a new API as proposed in #5522
_On a historical note, RecordBatchStream was itself a compromise, I originally wanted a push-based decoder https://github.com/apache/arrow-rs/issues/1605 but in the interests of time I went with the simpler, but more constraining option._
--
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