[GitHub] [arrow] pitrou commented on a change in pull request #9097: ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced

[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