alamb commented on code in PR #11144:
URL: https://github.com/apache/arrow-rs/pull/11144#discussion_r4506023241
##########
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:
Do we also have to check for Utf8?
##########
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:
this paragraph is pretty hard to read -- can you find some way to make ti
more concise?
Also the code does not check dictionaries so I am not sure what it is trying
to say
##########
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:
Can you give an example here of when the reader would map to a string type
rather than just restating?
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -53,6 +53,11 @@ pub fn make_byte_array_reader(
.clone(),
};
+ // 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::Utf8 |
ArrowType::LargeUtf8);
Review Comment:
This seems like the core check to me -- when the arrow type is
Utf8/LargeUtf8 then we need to validate utf8
##########
parquet/src/arrow/record_reader/mod.rs:
##########
@@ -230,6 +235,13 @@ where
self.padding_threshold = Some(threshold);
}
+ /// Require the values decoder to validate the data as UTF-8, for a column
Review Comment:
why does the record reader need to know this? Why isn't the check localized
to the values reader (which knows the resulting arrow type)?
##########
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:
This seems backwards -- the determination of validating UTF8 should be done
based on the type of the column. Maybe we need to store the value type instead
of validate_utf8?
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -197,6 +204,10 @@ impl<I: OffsetSizeTrait> ColumnValueDecoder for
ByteArrayColumnValueDecoder<I> {
}
}
+ fn set_validate_utf8(&mut self, validate_utf8: bool) {
+ self.validate_utf8 |= validate_utf8;
Review Comment:
Why |= ? Why not `=`?
##########
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:
Why do we need to document what the readers used to do? "the readers used to
enable ..."
##########
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:
Can you pleae update this test to use the ArrowWriter rather than the low
level SerializedFileWriter? It is quite verbose
##########
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:
On the other hand, I see this is just moved to a functions (the logic is
pre-existing) so it is ok
--
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]
