adamreeve commented on code in PR #50823:
URL: https://github.com/apache/arrow/pull/50823#discussion_r4332691459


##########
cpp/src/parquet/decoder.cc:
##########
@@ -2178,19 +2256,31 @@ class DeltaByteArrayDecoderImpl : public 
TypedDecoderImpl<DType> {
       last_value_in_previous_page_ = last_value_;
     }
 
-    if constexpr (std::is_same_v<DType, FLBAType>) {
-      // Checks all values
-      for (int i = 0; i < max_values; i++) {
-        if (buffer[i].len != static_cast<uint32_t>(this->type_length_)) {
-          throw ParquetException("FLBA type requires fixed-length ", 
this->type_length_,
-                                 " but got ", buffer[i].len);
-        }
-      }
-    }
-
     return max_values;
   }
 
+  int GetInternal(ByteArray* buffer, int max_values) {
+    ByteArrayOutput output{buffer, buffered_data_.get(), this->type_length_};
+    return GetInternalImpl(output, max_values);
+  }
+
+  int DecodeDense(uint8_t* out, int max_values) {
+    DenseOutput output{out, this->type_length_};
+    return GetInternalImpl(output, max_values);
+  }
+
+  int DecodePointers(FixedLenByteArray* buffer, int max_values) {

Review Comment:
   Nit: `DecodeFixedLenByteArray` might be a more suitable name. 



##########
cpp/src/parquet/decoder.cc:
##########
@@ -2178,19 +2256,31 @@ class DeltaByteArrayDecoderImpl : public 
TypedDecoderImpl<DType> {
       last_value_in_previous_page_ = last_value_;
     }
 
-    if constexpr (std::is_same_v<DType, FLBAType>) {
-      // Checks all values
-      for (int i = 0; i < max_values; i++) {
-        if (buffer[i].len != static_cast<uint32_t>(this->type_length_)) {
-          throw ParquetException("FLBA type requires fixed-length ", 
this->type_length_,
-                                 " but got ", buffer[i].len);
-        }
-      }
-    }
-
     return max_values;
   }
 
+  int GetInternal(ByteArray* buffer, int max_values) {

Review Comment:
   `DecodeArrow` still uses this path for the `FLBA` type, via 
`DecodeArrowDense`. Would it be possible to use `DecodeDense` for reading 
`FLBA` into Arrow, or would that be a much more complicated 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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to