hassaanch23 commented on code in PR #11144:
URL: https://github.com/apache/arrow-rs/pull/11144#discussion_r4506182571
##########
parquet/src/arrow/array_reader/byte_array_dictionary.rs:
##########
@@ -295,17 +308,33 @@ struct DictionaryDecoder<K, V> {
/// Dictionary decoder
decoder: Option<MaybeDictionaryDecoder>,
+ /// Whether the values are validated as UTF-8, and so built as strings,
+ /// see [`Self::value_type`].
validate_utf8: bool,
- value_type: ArrowType,
-
/// `Some` only for `FIXED_LEN_BYTE_ARRAY` columns, selecting fixed-width
/// decoding; `None` for other physical types.
flba_column_descr: Option<ColumnDescPtr>,
phantom: PhantomData<(K, V)>,
}
+impl<K, V: OffsetSizeTrait> DictionaryDecoder<K, V> {
+ /// The type of the dictionary values.
+ ///
+ /// Values are built as strings exactly when they are validated as UTF-8:
+ /// when the column is annotated as a string, or is read into a dictionary
+ /// with string values even though its annotation does not say so.
+ fn value_type(&self) -> ArrowType {
+ match (V::IS_LARGE, self.validate_utf8) {
Review Comment:
Right, the mapping itself is unchanged. It now reads the flag instead of the
annotation, so the values are built as strings exactly when they were
validated. That's what fixes `Dictionary<_, Utf8>` over an unannotated column,
which used to come back with `Binary` values.
##########
parquet/src/arrow/array_reader/byte_array_dictionary.rs:
##########
@@ -295,17 +308,33 @@ struct DictionaryDecoder<K, V> {
/// Dictionary decoder
decoder: Option<MaybeDictionaryDecoder>,
+ /// Whether the values are validated as UTF-8, and so built as strings,
+ /// see [`Self::value_type`].
validate_utf8: bool,
- value_type: ArrowType,
-
/// `Some` only for `FIXED_LEN_BYTE_ARRAY` columns, selecting fixed-width
/// decoding; `None` for other physical types.
flba_column_descr: Option<ColumnDescPtr>,
phantom: PhantomData<(K, V)>,
}
+impl<K, V: OffsetSizeTrait> DictionaryDecoder<K, V> {
+ /// The type of the dictionary values.
+ ///
+ /// Values are built as strings exactly when they are validated as UTF-8:
Review Comment:
Shortened in a2eb29e to: "The type of the dictionary values: strings if they
are validated as UTF-8, binary otherwise."
##########
parquet/src/arrow/array_reader/byte_view_array.rs:
##########
@@ -57,12 +57,18 @@ pub fn make_byte_view_array_reader(
},
};
+ // A schema given to the reader can map this column to a string type even
+ // though the Parquet annotation does not describe one, in which case the
Review Comment:
Done in a2eb29e. The comments in the three readers now give an example: a
schema passed to `ArrowReaderOptions::with_schema` reading an unannotated
`BYTE_ARRAY` column as `Utf8View` (or `Utf8`, or `Dictionary<_, Utf8>`).
##########
parquet/src/arrow/array_reader/byte_view_array.rs:
##########
@@ -57,12 +57,18 @@ pub fn make_byte_view_array_reader(
},
};
+ // A schema given to the reader can map this column to a string type even
+ // though the Parquet annotation does not describe one, in which case the
+ // decoder still has to validate the data as UTF-8.
+ let validate_utf8 = matches!(data_type, ArrowType::Utf8View);
Review Comment:
No. This reader only builds `BinaryView` and `Utf8View`, and any other type
returns an error a few lines below. `Utf8` and `LargeUtf8` go through
`make_byte_array_reader`, which checks both.
##########
parquet/src/arrow/arrow_reader/tests/schema.rs:
##########
@@ -599,66 +663,135 @@ fn test_read_binary_as_utf8() {
false,
),
Field::new("binary_view_to_utf8_view", ArrowDataType::Utf8View, false),
+ Field::new(
+ "binary_to_dictionary_utf8",
+ dictionary(ArrowDataType::Utf8),
+ false,
+ ),
+ Field::new(
+ "large_binary_to_dictionary_large_utf8",
+ dictionary(ArrowDataType::LargeUtf8),
+ false,
+ ),
+ Field::new(
+ "binary_to_dictionary_utf8_view",
+ dictionary(ArrowDataType::Utf8View),
+ false,
+ ),
]);
+ let supplied_schema = Arc::new(Schema::new(supplied_fields));
- let options =
ArrowReaderOptions::new().with_schema(Arc::new(Schema::new(supplied_fields)));
- let mut arrow_reader =
-
ParquetRecordBatchReaderBuilder::try_new_with_options(file.try_clone().unwrap(),
options)
+ for (encodings, props) in byte_array_encodings() {
+ let file = write_with_encodings(&batch, &encodings, props);
+ let options =
ArrowReaderOptions::new().with_schema(Arc::clone(&supplied_schema));
+ let mut arrow_reader =
ParquetRecordBatchReaderBuilder::try_new_with_options(file, options)
.expect("reader builder with schema")
.build()
.expect("reader with schema");
- let batch = arrow_reader.next().unwrap().unwrap();
- assert_eq!(batch.num_columns(), 3);
- assert_eq!(batch.num_rows(), 3);
- assert_eq!(
- batch
- .column(0)
- .as_string::<i32>()
- .iter()
- .collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
-
- assert_eq!(
- batch
- .column(1)
- .as_string::<i64>()
- .iter()
- .collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
-
- assert_eq!(
- batch.column(2).as_string_view().iter().collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
+ let read = arrow_reader.next().unwrap().unwrap();
+ assert_eq!(read.num_rows(), 3);
+ for (field, column) in
supplied_schema.fields().iter().zip(read.columns()) {
+ assert_eq!(column.data_type(), field.data_type(), "{encodings:?}");
+ column.to_data().validate_full().unwrap();
+ let strings = arrow_cast::cast(column,
&ArrowDataType::Utf8).unwrap();
+ assert_eq!(
+ strings.as_string::<i32>().iter().collect::<Vec<_>>(),
+ vec![Some("one"), Some("two"), Some("three")],
+ "{} written as {encodings:?}",
+ field.name()
+ );
+ }
+ }
}
+/// A supplied schema that reads a `Binary` column as a string type must still
+/// validate UTF-8. The readers used to enable validation only when the Parquet
Review Comment:
Removed in a2eb29e.
##########
parquet/src/arrow/arrow_reader/tests/schema.rs:
##########
@@ -599,66 +663,135 @@ fn test_read_binary_as_utf8() {
false,
),
Field::new("binary_view_to_utf8_view", ArrowDataType::Utf8View, false),
+ Field::new(
+ "binary_to_dictionary_utf8",
+ dictionary(ArrowDataType::Utf8),
+ false,
+ ),
+ Field::new(
+ "large_binary_to_dictionary_large_utf8",
+ dictionary(ArrowDataType::LargeUtf8),
+ false,
+ ),
+ Field::new(
+ "binary_to_dictionary_utf8_view",
+ dictionary(ArrowDataType::Utf8View),
+ false,
+ ),
]);
+ let supplied_schema = Arc::new(Schema::new(supplied_fields));
- let options =
ArrowReaderOptions::new().with_schema(Arc::new(Schema::new(supplied_fields)));
- let mut arrow_reader =
-
ParquetRecordBatchReaderBuilder::try_new_with_options(file.try_clone().unwrap(),
options)
+ for (encodings, props) in byte_array_encodings() {
+ let file = write_with_encodings(&batch, &encodings, props);
+ let options =
ArrowReaderOptions::new().with_schema(Arc::clone(&supplied_schema));
+ let mut arrow_reader =
ParquetRecordBatchReaderBuilder::try_new_with_options(file, options)
.expect("reader builder with schema")
.build()
.expect("reader with schema");
- let batch = arrow_reader.next().unwrap().unwrap();
- assert_eq!(batch.num_columns(), 3);
- assert_eq!(batch.num_rows(), 3);
- assert_eq!(
- batch
- .column(0)
- .as_string::<i32>()
- .iter()
- .collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
-
- assert_eq!(
- batch
- .column(1)
- .as_string::<i64>()
- .iter()
- .collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
-
- assert_eq!(
- batch.column(2).as_string_view().iter().collect::<Vec<_>>(),
- vec![Some("one"), Some("two"), Some("three")]
- );
+ let read = arrow_reader.next().unwrap().unwrap();
+ assert_eq!(read.num_rows(), 3);
+ for (field, column) in
supplied_schema.fields().iter().zip(read.columns()) {
+ assert_eq!(column.data_type(), field.data_type(), "{encodings:?}");
+ column.to_data().validate_full().unwrap();
+ let strings = arrow_cast::cast(column,
&ArrowDataType::Utf8).unwrap();
+ assert_eq!(
+ strings.as_string::<i32>().iter().collect::<Vec<_>>(),
+ vec![Some("one"), Some("two"), Some("three")],
+ "{} written as {encodings:?}",
+ field.name()
+ );
+ }
+ }
}
+/// A supplied schema that reads a `Binary` column as a string type must still
+/// validate UTF-8. The readers used to enable validation only when the Parquet
+/// column was annotated as a string, so a schema hint bypassed it entirely and
+/// produced a string array over arbitrary bytes.
#[test]
-#[should_panic(expected = "Invalid UTF8 sequence at")]
fn test_read_non_utf8_binary_as_utf8() {
- let file = write_parquet_from_iter(vec![(
+ // Invalid UTF-8 after an empty value and a null, so that when the writer
+ // abandons the dictionary after the first value, the invalid values are in
+ // the PLAIN pages that follow.
+ let batch = RecordBatch::try_from_iter(vec![(
"non_utf8_binary",
- Arc::new(BinaryArray::from(vec![
- b"\xDE\x00\xFF".as_ref(),
- b"\xDE\x01\xAA".as_ref(),
- b"\xDE\x02\xFF".as_ref(),
+ Arc::new(BinaryArray::from_opt_vec(vec![
+ Some(b"".as_ref()),
+ None,
+ Some(b"\xDE\x00\xFF".as_ref()),
+ Some(b"\xDE\x01\xAA".as_ref()),
])) as ArrayRef,
- )]);
- let supplied_fields = Fields::from(vec![Field::new(
- "non_utf8_binary",
- ArrowDataType::Utf8,
- false,
- )]);
+ )])
+ .unwrap();
+ let dictionary = |value_type| {
+ ArrowDataType::Dictionary(Box::new(ArrowDataType::Int32),
Box::new(value_type))
+ };
- let options =
ArrowReaderOptions::new().with_schema(Arc::new(Schema::new(supplied_fields)));
- let mut arrow_reader =
-
ParquetRecordBatchReaderBuilder::try_new_with_options(file.try_clone().unwrap(),
options)
+ for (encodings, props) in byte_array_encodings() {
+ let file = write_with_encodings(&batch, &encodings, props);
+ for supplied_type in [
+ ArrowDataType::Utf8,
+ ArrowDataType::LargeUtf8,
+ ArrowDataType::Utf8View,
+ dictionary(ArrowDataType::Utf8),
+ dictionary(ArrowDataType::LargeUtf8),
+ dictionary(ArrowDataType::Utf8View),
+ ] {
+ let supplied_fields = Fields::from(vec![Field::new(
+ "non_utf8_binary",
+ supplied_type.clone(),
+ true,
+ )]);
+
+ let options =
+
ArrowReaderOptions::new().with_schema(Arc::new(Schema::new(supplied_fields)));
+ let mut arrow_reader =
ParquetRecordBatchReaderBuilder::try_new_with_options(
+ file.try_clone().unwrap(),
+ options,
+ )
.expect("reader builder with schema")
.build()
.expect("reader with schema");
- arrow_reader.next().unwrap().unwrap_err();
+
+ let err = arrow_reader.next().unwrap().unwrap_err();
+ assert!(
+ err.to_string().contains("encountered non UTF-8 data"),
+ "{encodings:?} read as {supplied_type}: unexpected error:
{err}"
+ );
+ }
+ }
+}
+
+/// A `JSON` column is read as `Utf8` by default, so it must be validated as
+/// UTF-8 even though its converted type is `JSON` rather than `UTF8`.
+#[test]
+fn test_read_non_utf8_json() {
+ let message_type = "message schema { REQUIRED BYTE_ARRAY json (JSON); }";
Review Comment:
Done in a2eb29e. It now writes a `BinaryArray` with `ArrowWriter` and uses
`ArrowWriterOptions::with_parquet_schema` to give the column the `JSON`
annotation.
--
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]
