alamb commented on code in PR #11238:
URL: https://github.com/apache/arrow-rs/pull/11238#discussion_r4287833883
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -206,6 +207,73 @@ pub type ParquetPushDecoderBuilder =
ArrowReaderBuilder<PushDecoderInput>;
pub struct PushDecoderInput {
/// Bytes pushed into the decoder, awaiting decode.
buffers: PushBuffers,
+ /// How [`ParquetPushDecoder::try_decode`] fetches and decodes row groups.
+ fetch_granularity: FetchGranularity,
+}
+
+/// How [`ParquetPushDecoder::try_decode`] requests data and decodes row
+/// groups.
+///
+/// Set with [`ParquetPushDecoderBuilder::with_fetch_granularity`]. Both
+/// values give the same batches in the same order.
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum FetchGranularity {
+ /// Request and decode a full row group at a time. This is the default.
+ ///
+ /// [`DecodeResult::NeedsData`] asks for all bytes that the row group
+ /// reads, and the first batch is ready after all of them are pushed.
+ #[default]
+ RowGroup,
+ /// Request and decode one batch at a time.
+ ///
+ /// [`DecodeResult::NeedsData`] asks only for the pages that the next
+ /// batch reads, and the batch is ready after these pages are pushed. For
+ /// example, with a `batch_size` of 100 and pages of 100 rows, the first
+ /// request is for the dictionary page and the first data page of each
+ /// column.
+ ///
+ /// # Requirements
+ ///
+ /// * The offset index, to find the pages of a batch. Load it with
+ /// [`ArrowReaderOptions::with_page_index_policy`]. For a column chunk
+ /// without an offset index, the decoder requests the full column chunk.
+ /// * A `batch_size` greater than 0.
+ ///
+ /// # I/O
Review Comment:
This feels like the wrong place to document how to do I/O for the reader as
it is very deeply buried -- maybe we could move the documentation it to the
PushDecoder struct itself and leave a reference to that struct here?
Similarly, how the PushDecoder buffers / needs memory is probably something
we should put on the `PushDecoder` documentation itself rather than here on an
enum variant
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -206,6 +207,73 @@ pub type ParquetPushDecoderBuilder =
ArrowReaderBuilder<PushDecoderInput>;
pub struct PushDecoderInput {
/// Bytes pushed into the decoder, awaiting decode.
buffers: PushBuffers,
+ /// How [`ParquetPushDecoder::try_decode`] fetches and decodes row groups.
+ fetch_granularity: FetchGranularity,
+}
+
+/// How [`ParquetPushDecoder::try_decode`] requests data and decodes row
+/// groups.
+///
+/// Set with [`ParquetPushDecoderBuilder::with_fetch_granularity`]. Both
+/// values give the same batches in the same order.
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum FetchGranularity {
+ /// Request and decode a full row group at a time. This is the default.
+ ///
+ /// [`DecodeResult::NeedsData`] asks for all bytes that the row group
+ /// reads, and the first batch is ready after all of them are pushed.
Review Comment:
maybe we can say this offers good decode speed but will buffer all the pages
needed for decode in the row group; For larger row groups (e.g. that have many
strings) this can may require substantial memory
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -206,6 +207,73 @@ pub type ParquetPushDecoderBuilder =
ArrowReaderBuilder<PushDecoderInput>;
pub struct PushDecoderInput {
/// Bytes pushed into the decoder, awaiting decode.
buffers: PushBuffers,
+ /// How [`ParquetPushDecoder::try_decode`] fetches and decodes row groups.
+ fetch_granularity: FetchGranularity,
+}
+
+/// How [`ParquetPushDecoder::try_decode`] requests data and decodes row
Review Comment:
# Naming:
The push decoder isn't actually 'fetching' anything, so calling this
enum`FetchGranularity` feels strange to me. Maybe using something more
connected to request or NeedsData would be more natural? Perhaps
- `NeedsDataGranularity`
- `RequestGranularity`
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -206,6 +207,73 @@ pub type ParquetPushDecoderBuilder =
ArrowReaderBuilder<PushDecoderInput>;
pub struct PushDecoderInput {
/// Bytes pushed into the decoder, awaiting decode.
buffers: PushBuffers,
+ /// How [`ParquetPushDecoder::try_decode`] fetches and decodes row groups.
+ fetch_granularity: FetchGranularity,
+}
+
+/// How [`ParquetPushDecoder::try_decode`] requests data and decodes row
+/// groups.
+///
+/// Set with [`ParquetPushDecoderBuilder::with_fetch_granularity`]. Both
+/// values give the same batches in the same order.
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum FetchGranularity {
+ /// Request and decode a full row group at a time. This is the default.
+ ///
+ /// [`DecodeResult::NeedsData`] asks for all bytes that the row group
+ /// reads, and the first batch is ready after all of them are pushed.
+ #[default]
+ RowGroup,
+ /// Request and decode one batch at a time.
+ ///
+ /// [`DecodeResult::NeedsData`] asks only for the pages that the next
+ /// batch reads, and the batch is ready after these pages are pushed. For
+ /// example, with a `batch_size` of 100 and pages of 100 rows, the first
+ /// request is for the dictionary page and the first data page of each
+ /// column.
+ ///
+ /// # Requirements
+ ///
+ /// * The offset index, to find the pages of a batch. Load it with
+ /// [`ArrowReaderOptions::with_page_index_policy`]. For a column chunk
+ /// without an offset index, the decoder requests the full column chunk.
+ /// * A `batch_size` greater than 0.
Review Comment:
I don't think a batch_size of zero is common and I doubt the decoder will
work with it (if so we should probably reject it).
Also, I think we could potentially clarify this a bit like
```suggestion
/// * The [`ParquetMetaData`] must have an offset index, for all
requested columns
/// to find the pages of a batch (see
/// [`ArrowReaderOptions::with_page_index_policy`]). The decoder will
request the full
/// column chunk for all columns without an offset index.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
