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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
