[GitHub] [arrow] wgtmac commented on a diff in pull request #35825: GH-32723: [C++][Parquet] Add option to use LARGE* variants of binary types

[via GitHub]

wgtmac commented on code in PR #35825:
URL: https://github.com/apache/arrow/pull/35825#discussion_r1226096028


##########
cpp/src/parquet/types.h:
##########
@@ -761,6 +761,14 @@ using Int96Type = PhysicalType<Type::INT96>;
 using FloatType = PhysicalType<Type::FLOAT>;
 using DoubleType = PhysicalType<Type::DOUBLE>;
 using ByteArrayType = PhysicalType<Type::BYTE_ARRAY>;
+
+/*
+ * Parquet uses ByteArrayType for variable length strings and binaries and 
their lengths
+ * will not exceed 2^31 - 1. However, arrow supports StringType/BinaryType and 
their
+ * large variants (i.e. LargeStringType and LargeBinaryType).
+ * */

Review Comment:
   ```suggestion
   /**
    * Parquet has defined ByteArrayType for variable length string and binary 
values with a
    * maximum length of 2^31 - 1. By default, arrow StringType and BinaryType 
are used to
    * map parquet ByteArrayType. However, arrow StringArray/BinaryArray uses 
int32_t to
    * store the offset of each string/binary value in a concatenated buffer 
which may
    * overflow (though unlikely in most cases). As arrow has defined 
LargeStringType and
    * LargeBinaryType which use int64_t as the offset type, we define 
LargeByteArrayType
    * below to indicate parquet reader/writer to use those large variants from 
arrow.
    */
   ```



##########
cpp/src/parquet/encoding.cc:
##########
@@ -1484,6 +1521,37 @@ class DictDecoderImpl : public DecoderImpl, virtual 
public DictDecoder<Type> {
   // Perform type-specific initiatialization
   void SetDict(TypedDecoder<Type>* dictionary) override;
 
+  template <typename T = Type,
+            typename = std::enable_if_t<std::is_same_v<T, ByteArrayType> ||
+                                        std::is_same_v<T, LargeByteArrayType>>>
+  void SetByteArrayDict(TypedDecoder<Type>* dictionary) {
+    DecodeDict(dictionary);
+
+    auto dict_values = 
reinterpret_cast<ByteArray*>(dictionary_->mutable_data());
+
+    int total_size = 0;

Review Comment:
   This now looks fine to me as checked in line 1535.



##########
cpp/src/parquet/encoding.cc:
##########
@@ -1484,6 +1521,39 @@ class DictDecoderImpl : public DecoderImpl, virtual 
public DictDecoder<Type> {
   // Perform type-specific initiatialization
   void SetDict(TypedDecoder<Type>* dictionary) override;
 
+  template <typename T = Type,
+            typename = std::enable_if_t<std::is_same_v<T, ByteArrayType> ||
+                                        std::is_same_v<T, LargeByteArrayType>>>
+  void SetByteArrayDict(TypedDecoder<Type>* dictionary) {
+    DecodeDict(dictionary);
+
+    auto dict_values = 
reinterpret_cast<ByteArray*>(dictionary_->mutable_data());
+
+    int total_size = 0;
+    for (int i = 0; i < dictionary_length_; ++i) {
+      if (AddWithOverflow(total_size, dict_values[i].len, &total_size)) {
+        throw ParquetException("String/Binary Length to large");

Review Comment:
   ```suggestion
           throw ParquetException("String/Binary length too large");
   ```



-- 
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-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org