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:
us...@infra.apache.org
