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/01/04 21:14:05 UTC
[GitHub] [arrow] xhochy opened a new pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
xhochy opened a new pull request #9097:
URL: https://github.com/apache/arrow/pull/9097
----------------------------------------------------------------
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] xhochy commented on pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
xhochy commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754872419
@pitrou 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#discussion_r551874341
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -110,11 +110,16 @@ class PlainEncoder : public EncoderImpl, virtual public TypedEncoder<DType> {
void PutSpaced(const T* src, int num_values, const uint8_t* valid_bits,
int64_t valid_bits_offset) override {
- PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
- this->memory_pool()));
- T* data = reinterpret_cast<T*>(buffer->mutable_data());
- int num_valid_values = ::arrow::util::internal::SpacedCompress<T>(
- src, num_values, valid_bits, valid_bits_offset, data);
+ const T* data = src;
+ int num_valid_values = num_values;
+ if (valid_bits != NULLPTR) {
+ PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
+ this->memory_pool()));
Review comment:
It seems `buffer` will be deallocated at the end of this `if` block, but you're using its contents afterwards (when calling `Put`).
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -110,11 +110,16 @@ class PlainEncoder : public EncoderImpl, virtual public TypedEncoder<DType> {
void PutSpaced(const T* src, int num_values, const uint8_t* valid_bits,
int64_t valid_bits_offset) override {
- PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
- this->memory_pool()));
- T* data = reinterpret_cast<T*>(buffer->mutable_data());
- int num_valid_values = ::arrow::util::internal::SpacedCompress<T>(
- src, num_values, valid_bits, valid_bits_offset, data);
+ const T* data = src;
+ int num_valid_values = num_values;
+ if (valid_bits != NULLPTR) {
+ PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
+ this->memory_pool()));
+ T* data_ = reinterpret_cast<T*>(buffer->mutable_data());
Review comment:
Underscore-terminated variable names are only for attributes. Can you use another name? (e.g. `non_spaced_data`)
##########
File path: cpp/src/parquet/encoding.cc
##########
@@ -309,11 +314,16 @@ class PlainEncoder<BooleanType> : public EncoderImpl, virtual public BooleanEnco
void PutSpaced(const bool* src, int num_values, const uint8_t* valid_bits,
int64_t valid_bits_offset) override {
- PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
- this->memory_pool()));
- T* data = reinterpret_cast<T*>(buffer->mutable_data());
- int num_valid_values = ::arrow::util::internal::SpacedCompress<T>(
- src, num_values, valid_bits, valid_bits_offset, data);
+ const T* data = src;
+ int num_valid_values = num_values;
+ if (valid_bits != NULLPTR) {
+ PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
Review comment:
Same concerns below.
##########
File path: cpp/src/parquet/encoding.h
##########
@@ -290,14 +290,18 @@ class TypedDecoder : virtual public Decoder {
/// \return The number of values decoded, including nulls.
virtual int DecodeSpaced(T* buffer, int num_values, int null_count,
const uint8_t* valid_bits, int64_t valid_bits_offset) {
- int values_to_read = num_values - null_count;
- int values_read = Decode(buffer, values_to_read);
- if (values_read != values_to_read) {
- throw ParquetException("Number of values / definition_levels read did not match");
+ if (null_count > 0) {
+ int values_to_read = num_values - null_count;
+ int values_read = Decode(buffer, values_to_read);
+ if (values_read != values_to_read) {
+ throw ParquetException("Number of values / definition_levels read did not match");
+ }
+
+ return ::arrow::util::internal::SpacedExpand<T>(buffer, num_values, null_count,
+ valid_bits, valid_bits_offset);
+ } else {
+ return num_values;
Review comment:
Why don't you call `Decode` in this branch?
----------------------------------------------------------------
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] xhochy commented on pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
xhochy commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754256374
@github-actions autotune
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754222849
https://issues.apache.org/jira/browse/ARROW-10881
----------------------------------------------------------------
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] nealrichardson commented on pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754297915
@xhochy autotune isn't working at the moment--INFRA has blocked some of the Actions we use, including on that workflow. See https://issues.apache.org/jira/browse/INFRA-21239
----------------------------------------------------------------
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 #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #9097:
URL: https://github.com/apache/arrow/pull/9097
----------------------------------------------------------------
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 #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754828459
CI tests are green on my fork (except for the usual Homebrew dependency install failures).
----------------------------------------------------------------
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 #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9097:
URL: https://github.com/apache/arrow/pull/9097#issuecomment-754254123
Is it possible to add 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