You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/02/06 08:13:08 UTC
[GitHub] [arrow] diggerk opened a new pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
diggerk opened a new pull request #9432:
URL: https://github.com/apache/arrow/pull/9432
Parquet reader crashes while reading boolean columns due to incorrect
calculation of buffer size in TypedColumnReaderImpl::Skip. This change
fixes the buffer size calculation to accomodate levels data.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation when reading/skipping boolean columns
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-887567925
Thank you @diggerk and sorry 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
[GitHub] [arrow] diggerk commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
diggerk commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-780127603
@pitrou , adding a unit test turned out to be harder than expected. I had to add support for booleans to the class used for parquet reader testing. The new test doesn't fail without the fix unless I run it with a large number of repetitions, and even then it hangs instead of failing. I think it's just hard to reproduce it in a better way due to the nature of the problem with memory allocation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#discussion_r572032957
##########
File path: cpp/src/parquet/column_reader.cc
##########
@@ -1010,8 +1010,9 @@ int64_t TypedColumnReaderImpl<DType>::Skip(int64_t num_rows_to_skip) {
// This will be enough scratch space to accommodate 16-bit levels or any
// value type
+ int16_t value_size = type_traits<DType::type_num>::value_byte_size;
Review comment:
Why `int16_t`? This should probably be `int`.
##########
File path: cpp/src/parquet/column_reader.cc
##########
@@ -1010,8 +1010,9 @@ int64_t TypedColumnReaderImpl<DType>::Skip(int64_t num_rows_to_skip) {
// This will be enough scratch space to accommodate 16-bit levels or any
// value type
+ int16_t value_size = type_traits<DType::type_num>::value_byte_size;
std::shared_ptr<ResizableBuffer> scratch = AllocateBuffer(
- this->pool_, batch_size * type_traits<DType::type_num>::value_byte_size);
+ this->pool_, batch_size * std::max<int16_t>(sizeof(int16_t), value_size));
Review comment:
Similarly, `std::max<int>`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation when reading/skipping boolean columns
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #9432:
URL: https://github.com/apache/arrow/pull/9432
--
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] lidavidm commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-880868577
Looks like this needs rebasing now.
--
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 #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-774422926
https://issues.apache.org/jira/browse/ARROW-11518
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-774543891
Thank you for the pr. Would you mind adding a unit test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-780580016
@diggerk Thanks for the update. I'll take a look again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#discussion_r577666626
##########
File path: cpp/src/parquet/column_reader_test.cc
##########
@@ -71,23 +71,30 @@ static inline bool vector_equal_with_def_levels(const std::vector<T>& left,
return true;
}
+template <typename Type>
class TestPrimitiveReader : public ::testing::Test {
public:
+ using c_type = typename Type::c_type;
+
void InitReader(const ColumnDescriptor* d) {
std::unique_ptr<PageReader> pager_;
pager_.reset(new test::MockPageReader(pages_));
reader_ = ColumnReader::Make(d, std::move(pager_));
}
void CheckResults() {
- std::vector<int32_t> vresult(num_values_, -1);
+ std::vector<c_type> vresult(num_values_, -1);
+ std::vector<uint8_t> vresult_bool(num_values_, -1);
+ c_type* vresult_ptr = ResultsPointer(vresult, vresult_bool);
Review comment:
It looks like the `PrimitiveTypedTest` class in `test_util.h` already has the necessary harnessing around `vector<bool> <-> vector<uint8_t>`. Perhaps you can try reusing it instead of reinventing similar routines here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #9432: ARROW-11518: [C++][Parquet] Fix buffer allocation in parquet reader
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #9432:
URL: https://github.com/apache/arrow/pull/9432#issuecomment-887457837
I've rebased and I took the decision to revert the test additions. While it would be nice to unit test this, the current Parquet test harness is basically unmaintainable, and I'd rather not add more write-only code there.
--
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