You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jp0317 (via GitHub)" <gi...@apache.org> on 2023/07/06 19:57:20 UTC
[GitHub] [arrow] jp0317 opened a new pull request, #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
jp0317 opened a new pull request, #36510:
URL: https://github.com/apache/arrow/pull/36510
### Rationale for this change
When buffered stream is enabled, all column chunks, regardless of their actual sizes, are currently sharing the same buffer size which is stored in the shared [read properties](https://github.com/apache/arrow/blob/main/cpp/src/parquet/file_reader.cc#L213).
Given a limited memory budget, one may want to customize buffer size for different column chunks based on their actual size, i.e., smaller chunks will use consume less memory budget for its buffer.
### What changes are included in this PR?
Codes for customizing buffer size and unit tests
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Extend some APIs on file_reader.h with a new parameter defining the customized buffer size for a column chunk. The default behavior (if the new parameter is not specified) will not 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
[GitHub] [arrow] jp0317 commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1627768067
Thanks for the comments. Would allowing an optional column-specific read properties instead of optional buffer size be better? This column-specific read properties can be the `ColumnReaderProperties` as mapleFU suggested which is only about buffer size, or just the `ReadProperties` which covers [more settings](https://github.com/apache/arrow/blob/main/cpp/src/parquet/properties.h#L120C1-L126). Given that we already let users decide the global buffer size, leaving column-specific option to users might be more feasible and easier than tuning internally inside file reader. And users can still choose to stick with the old behavior (using global buffer size) if column-specific is difficult/unimportant to them.
Thanks mapleFu@ for the suggestion. It does look better. It seems to introduce new API to set and store the column specific buffer size before creating column page reader (e.g., before calling `Column(int i)`), and the read properties should be aware of the target column index when creating the stream buffer. If embedding the column specific properties in the creation API as mentioned above be simpler?
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1262829199
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Thanks for the review. Changed to `ColumnReaderProperties`.
>would already has properties_
the `properties_` will be shared by all columns in the row group reader. I was thinking adding a column specific properties.
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1257275770
##########
cpp/src/parquet/properties.h:
##########
@@ -64,7 +64,8 @@ class PARQUET_EXPORT ReaderProperties {
MemoryPool* memory_pool() const { return pool_; }
std::shared_ptr<ArrowInputStream> GetStream(std::shared_ptr<ArrowInputFile> source,
- int64_t start, int64_t num_bytes);
+ int64_t start, int64_t num_bytes,
+ int64_t buffer_size = -1);
Review Comment:
What about `std::optional<int64_t> buffer_size`?
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,7 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(int i, int64_t buffer_size) {
Review Comment:
TBH, this additional parameter looks a little bit weird here.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -189,6 +190,9 @@ class PARQUET_EXPORT ParquetFileReader {
::arrow::Future<> WhenBuffered(const std::vector<int>& row_groups,
const std::vector<int>& column_indices) const;
+ /// Return the range of the specified column chunk.
+ ::arrow::io::ReadRange GetColumnChunkRange(int row_group_index, int column_index);
Review Comment:
This is not used any where?
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(int i,
Review Comment:
I didn't see how the file reader deal with this new parameter. Is it intended for the caller to pass a good buffer_size?
--
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] mapleFU commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1627576815
Personally I think create column chunk with a buffer size is tricky. And maybe something like:
```
class ColumnReaderProperties {
void set_buffer_size(int64_t);
int64_t buffer_size();
};
class ReaderProperties {
// default?
int64_t buffer_size();
//
int64_t buffer_size(int column_id);
private:
int64_t default_buffer_size_;
// col ->
std::map<int, ColumnReaderProperties> column_properties_;
};
```
would this make code better for 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
[GitHub] [arrow] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1296490760
##########
cpp/src/parquet/properties.h:
##########
@@ -56,15 +57,28 @@ constexpr int32_t kDefaultThriftStringSizeLimit = 100 * 1000 * 1000;
// kDefaultStringSizeLimit.
constexpr int32_t kDefaultThriftContainerSizeLimit = 1000 * 1000;
+class PARQUET_EXPORT ColumnReaderProperties {
+ public:
+ void set_buffer_size(int64_t buffer_size) { buffer_size_ = buffer_size; }
+ int64_t buffer_size() const { return buffer_size_; }
+
+ private:
+ int64_t buffer_size_ = kDefaultBufferSize;
+};
+
class PARQUET_EXPORT ReaderProperties {
public:
+ typedef std::unordered_map<int, std::unordered_map<int, ColumnReaderProperties>>
+ ColumnReaderPropertiesMap;
+
explicit ReaderProperties(MemoryPool* pool = ::arrow::default_memory_pool())
: pool_(pool) {}
MemoryPool* memory_pool() const { return pool_; }
- std::shared_ptr<ArrowInputStream> GetStream(std::shared_ptr<ArrowInputFile> source,
- int64_t start, int64_t num_bytes);
+ std::shared_ptr<ArrowInputStream> GetStream(
+ std::shared_ptr<ArrowInputFile> source, int64_t start, int64_t num_bytes,
+ std::optional<int64_t> buffer_size = std::nullopt);
Review Comment:
Using column/rowgroup index in `GetStream` seems a little bit weird to me...iiuc this function is to get a stream for the specific data range, which is not necessarily a column chunk
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1265621368
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Thanks for the comment! I feel that would require more changes on the `properties_` related logic. IIUC the `properties_` is passed to file reader during its creation and is copied to each row group reader. In the context of column chunk specific buffer size, a row group reader does not need a `properties_` that stores `ColumnReaderProperties` for an irrelevant row group. Besides, maintaining such a map might not be cheap when there're numerous columns (in another PR i'm changing a map to bit vector). Another way, which i feel is less cleaner, is to provide new apis allowing users to update the `properties_`, such that the `properties_` contains the `ColumnReaderProperties` only for the current row group reader being created.
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1282676014
##########
cpp/src/parquet/properties.h:
##########
@@ -79,10 +93,45 @@ class PARQUET_EXPORT ReaderProperties {
/// Disable buffered stream reading.
void disable_buffered_stream() { buffered_stream_enabled_ = false; }
- /// Return the size of the buffered stream buffer.
- int64_t buffer_size() const { return buffer_size_; }
- /// Set the size of the buffered stream buffer in bytes.
- void set_buffer_size(int64_t size) { buffer_size_ = size; }
+ /// Return the default size of the buffered stream buffer.
+ int64_t buffer_size() const { return default_column_reader_properties_.buffer_size(); }
+ /// Set the default size of the buffered stream buffer in bytes.
+ void set_buffer_size(int64_t size) {
+ default_column_reader_properties_.set_buffer_size(size);
+ }
+
+ /// Return the size of the buffered stream buffer for a column chunk.
+ int64_t buffer_size(int row_group_index, int column_index) const {
Review Comment:
The current change looks better than leaking the property thing into the reader api. However, I'm not sure if it is appropriate to introduce concept like `row_group_index` into ReaderProperties, especially when `ColumnReaderPropertiesMap` is organized into a two-layer design. Sorry but I would -1 on making such change on optimization like this.
WDYT? @pitrou @emkornfield @mapleFU
--
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] jp0317 commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1627482654
> Thanks for making a PR! However, I am really in doubt about its effectiveness. As the buffered stream does not know the page boundary, it can only issue next read in its best effort. As the caller, we don't know the page size either to tune the buffer_size here. IMO, the best solution is to equip PageReader with page index if available, and plan the read range based on the offset index of each page. WDYT?
Thanks for your review! I agree with the page offset but feel it might be different topic. The new `buffer_size` parameter in this PR is to allow users to customize buffer size for different column chunks, while currently all column chunks have to share the same buffer size from `read_properties`. On users side, they used to set a single buffer size for `read_properties`, with this PR they can further choose to customize a buffer size for specific column chunk. E.g., if the user wants to limit the buffer memory to 64Mb when reading two chunks with 10MB and 100MB size, they now can assign 10Mb for the smaller chunk and the rest 54Mb for the larger one.
--
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] PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader [arrow]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 closed pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
URL: https://github.com/apache/arrow/pull/36510
--
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] jp0317 commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1681338986
> Let's step back a bit here. You are talking about limited memory use cases. The default buffer size is `kDefaultBufferSize = 4096 * 4`. Is there a situation where this is too large?
Thanks for the review! Generally 16kb is not large. I think the issue is about whether it pays to fully take advantage of given memory budget and support a column specific or column chunk specific buffer size, as discussed [here](https://github.com/apache/arrow/pull/36510#issuecomment-1627482654).
--
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] mapleFU commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1262276193
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Seems that `RowGroupReader` would already has `properties_`. Could you use 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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1257354094
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(int i,
Review Comment:
Yes, it's intended. And i think the caller behavior is almost the same: the only difference is that they now have one more option to customize the buffer size per column chunk, in addition to setting the [read properties](https://github.com/apache/arrow/blob/main/cpp/src/parquet/properties.h#L121) shared by all chunks.
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1262262856
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
This does not look better than the proposed `ColumnProperties` from @mapleFU
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1265592288
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
but if that can also be inaccurate, wouldn't the current `Prebuffer()` be problematic as it [relies also on](https://github.com/apache/arrow/blob/main/cpp/src/parquet/file_reader.cc#L373) the chunk range?
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1264879576
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
`GetColumnChunkRange` is just an internal implementation to roughly get the read range of column chunk. In fact, the exact range of a column chunk can be more complex if IndexPage and ColumnMetaData are considered. It would be a burden to make it public.
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1267015349
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Is it an overkill to setup a column-level buffer size of each row group? If you really need control in this granularity, you can create one reader for each row group. Probably enabling buffered stream in this case is not a good idea as its behavior is not that predicable.
ReaderProperties is immutable once reader has been created, mutating it seems not a good idea to me.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1257354575
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,7 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(int i, int64_t buffer_size) {
Review Comment:
yeah..it's added for those who want to control stream buffer size. For others they can keep the default value nullopt
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1265591498
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
ah..i see.
--
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] mapleFU commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1299903917
##########
cpp/src/parquet/properties.h:
##########
@@ -79,10 +93,45 @@ class PARQUET_EXPORT ReaderProperties {
/// Disable buffered stream reading.
void disable_buffered_stream() { buffered_stream_enabled_ = false; }
- /// Return the size of the buffered stream buffer.
- int64_t buffer_size() const { return buffer_size_; }
- /// Set the size of the buffered stream buffer in bytes.
- void set_buffer_size(int64_t size) { buffer_size_ = size; }
+ /// Return the default size of the buffered stream buffer.
+ int64_t buffer_size() const { return default_column_reader_properties_.buffer_size(); }
+ /// Set the default size of the buffered stream buffer in bytes.
+ void set_buffer_size(int64_t size) {
+ default_column_reader_properties_.set_buffer_size(size);
+ }
+
+ /// Return the size of the buffered stream buffer for a column chunk.
+ int64_t buffer_size(int row_group_index, int column_index) const {
Review Comment:
Personally I think deciding a buffer-size for per-row-group and per-column is to hack... Perhaps user here has define a `CalculateBufferSize(const ColumnChunkMetadata&)`, and caculating this.
it's ok but I guess it's too complex... Why a consistent buffer_size for each column is not ok? cc @pitrou
--
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] wgtmac commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1627572329
Yep, I understand your overall goal is to control the total memory used in the buffered stream.
My main concern is that parameters like `buffer_size` which should be solely controlled by ReaderProperties now it is at large and runs away into the public API of reader. It is yet not easy for end users to tune the new parameter: e.g. they need to call `ComputeColumnChunkRange` that you have just exposed to get a basic idea of column chunk size and make a decision based on that. Moreover, people in the future may follow the same way to implement their hacky optimizations which tend to make the code base difficult to maintain. It seems to me the benefit it brings does not worth breaking the design.
It would be more favorable if we can add a new setting to ReaderProperties to fine tune the behavior and let the file reader to optimize this internally without changing any public API on the reader side.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1262829199
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Thanks for the review. Changed to `ColumnReaderProperties`.
>would already has properties_
the `properties_` will be shared by all columns in the row group reader. I was thinking adding a column specific properties.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1265592288
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
but if that can also be inaccurate, wouldn't the current `Prebuffer()` be problematic as it relies also on the chunk range?
--
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] github-actions[bot] commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1624236002
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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] github-actions[bot] commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1624235968
https://issues.apache.org/jira/browse/PARQUET-2321
--
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] mapleFU commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1299901798
##########
cpp/src/parquet/properties.cc:
##########
@@ -28,16 +28,20 @@
namespace parquet {
std::shared_ptr<ArrowInputStream> ReaderProperties::GetStream(
- std::shared_ptr<ArrowInputFile> source, int64_t start, int64_t num_bytes) {
- if (buffered_stream_enabled_) {
+ std::shared_ptr<ArrowInputFile> source, int64_t start, int64_t num_bytes,
+ std::optional<int64_t> custom_buffer_size) {
Review Comment:
Emmm why do we need a `custom_buffer_size` even when we already have a `buffer_size` internal? This makes building a stream has "default buffer size", "buffer size in ReaderProperties", "buffer size in GetStream"...
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1257353049
##########
cpp/src/parquet/file_reader.h:
##########
@@ -189,6 +190,9 @@ class PARQUET_EXPORT ParquetFileReader {
::arrow::Future<> WhenBuffered(const std::vector<int>& row_groups,
const std::vector<int>& column_indices) const;
+ /// Return the range of the specified column chunk.
+ ::arrow::io::ReadRange GetColumnChunkRange(int row_group_index, int column_index);
Review Comment:
yes, it's just exposing the internal `ComputeColumnChunkRange` to users such that they can know the actual column chunks size.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1270183127
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
sure, i can make a copy of it given those concerns. Thanks.
--
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] mapleFU commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1282680155
##########
cpp/src/parquet/properties.h:
##########
@@ -56,15 +57,28 @@ constexpr int32_t kDefaultThriftStringSizeLimit = 100 * 1000 * 1000;
// kDefaultStringSizeLimit.
constexpr int32_t kDefaultThriftContainerSizeLimit = 1000 * 1000;
+class PARQUET_EXPORT ColumnReaderProperties {
+ public:
+ void set_buffer_size(int64_t buffer_size) { buffer_size_ = buffer_size; }
+ int64_t buffer_size() const { return buffer_size_; }
+
+ private:
+ int64_t buffer_size_ = kDefaultBufferSize;
+};
+
class PARQUET_EXPORT ReaderProperties {
public:
+ typedef std::unordered_map<int, std::unordered_map<int, ColumnReaderProperties>>
+ ColumnReaderPropertiesMap;
+
explicit ReaderProperties(MemoryPool* pool = ::arrow::default_memory_pool())
: pool_(pool) {}
MemoryPool* memory_pool() const { return pool_; }
- std::shared_ptr<ArrowInputStream> GetStream(std::shared_ptr<ArrowInputFile> source,
- int64_t start, int64_t num_bytes);
+ std::shared_ptr<ArrowInputStream> GetStream(
+ std::shared_ptr<ArrowInputFile> source, int64_t start, int64_t num_bytes,
+ std::optional<int64_t> buffer_size = std::nullopt);
Review Comment:
why it need a input `buffer_size` when having a PropertiesMap?
--
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] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1267003756
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
The Prebuffer() only requires to read dictionary and data pages. It does not need to read ColumnMetaData and cannot read any customized index page. Based on that assumption, the current implementation can satisfy its need. However, if we expose the logic to external user, it is not easy to make it clear what is the total range of a column chunk (even the specs does not provide a good approach to get it right). I know you need that information to provide hints to estimate the read boundary of a column chunk. You can simply copy the code into your own business logic. It would be way easier to maintain on your own. WDYT?
--
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] PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader [arrow]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1803075090
sorry for the late update. I think we can close this one for now as it may not worth making all these changes for a certain scenario. I'll revisit this if it became more favorable. Thanks for all the comments and reviews.
--
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] pitrou commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1681747637
> I think the issue is about whether it pays to fully take advantage of given memory budget and support a column specific or column chunk specific buffer size, as discussed [here](https://github.com/apache/arrow/pull/36510#issuecomment-1627482654).
My intuitive answer is no. But perhaps you can run benchmarks to get a better idea.
--
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] pitrou commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1677625097
Let's step back a bit here. You are talking about limited memory use cases. The default buffer size is `kDefaultBufferSize = 4096 * 4`. Is there a situation where this is too large?
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1274053645
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
Thanks for the review. I put all `ColumnReaderProperties` in the `ReaderProperties` as a shared pointer. Could you please take a look at it? Thanks.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1257350451
##########
cpp/src/parquet/properties.h:
##########
@@ -64,7 +64,8 @@ class PARQUET_EXPORT ReaderProperties {
MemoryPool* memory_pool() const { return pool_; }
std::shared_ptr<ArrowInputStream> GetStream(std::shared_ptr<ArrowInputFile> source,
- int64_t start, int64_t num_bytes);
+ int64_t start, int64_t num_bytes,
+ int64_t buffer_size = -1);
Review Comment:
thanks for the suggestion, changed 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
[GitHub] [arrow] wgtmac commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1264696705
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
It is still not a good practice to add a new ColumnReaderProperties parameter here. We should get all reader properties (whatever file level or column level) via the `properties_` object. Could you simply copy the behavior of `WriterProperties` and `ColumnProperties`, then do something similar to `ReaderProperties` and `ColumnReaderProperties`? The gap is that ReaderProperties does not provide a builder pattern and directly implements `set_xxx()` functions instead. We can add `ReaderProperties::set_buffer_size(const std::string& path, int64_t size)` (and the schema::ColumnPath variant), and put the size value into the internal `std::map<std::string, ColumnReaderProperties>`.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -189,6 +190,9 @@ class PARQUET_EXPORT ParquetFileReader {
::arrow::Future<> WhenBuffered(const std::vector<int>& row_groups,
const std::vector<int>& column_indices) const;
+ /// Return the range of the specified column chunk.
+ ::arrow::io::ReadRange GetColumnChunkRange(int row_group_index, int column_index);
Review Comment:
I'd rather not exposing this function externally.
--
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] jp0317 commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1264734613
##########
cpp/src/parquet/file_reader.h:
##########
@@ -44,7 +44,8 @@ class PARQUET_EXPORT RowGroupReader {
// An implementation of the Contents class is defined in the .cc file
struct Contents {
virtual ~Contents() {}
- virtual std::unique_ptr<PageReader> GetColumnPageReader(int i) = 0;
+ virtual std::unique_ptr<PageReader> GetColumnPageReader(
Review Comment:
Thanks for the review. Regarding `GetColumnChunkRange`, is there any concern exposing it? IIUC currently users can only rely on `total_compressed_size` which reveals no offset information and may not reflect the actual chunk size .
For the `ColumnReaderProperties`, given that the reader apis are all index based, maybe we can just use index (as mapleFU suggested) without involving column paths, especially a map on path strings? Initially i was trying to avoid keeping such a map in `ReaderProperties`, and more importantly, i feel it makes sense to implement this customized buffer size as "column chunk specific": different column chunks from the same column can have different buffer size.
--
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] mapleFU commented on a diff in pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36510:
URL: https://github.com/apache/arrow/pull/36510#discussion_r1264869980
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -66,7 +66,8 @@ static constexpr int64_t kMaxDictHeaderSize = 100;
RowGroupReader::RowGroupReader(std::unique_ptr<Contents> contents)
: contents_(std::move(contents)) {}
-std::shared_ptr<ColumnReader> RowGroupReader::Column(int i) {
+std::shared_ptr<ColumnReader> RowGroupReader::Column(
+ int i, std::optional<ReaderProperties> prop) {
Review Comment:
@jp0317 In `WriterProperties`, it has a Column-specific writer property, and when column-level property doesn't exist, it will refer to original property. Why not just use that to handling?
And under that, `Column` will get `buffer_size()` without breaking the previous interface.
--
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] jp0317 commented on pull request #36510: PARQUET-2321: [C++] allow customized buffer size when creating ArrowInputStream for a column PageReader
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #36510:
URL: https://github.com/apache/arrow/pull/36510#issuecomment-1631728992
Thanks for the above suggestions. I changed to pass a `ReadProperties` instead of a raw `buffer_size` when creating column/page reader. PTAL.
--
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