hassaanch23 commented on code in PR #11144:
URL: https://github.com/apache/arrow-rs/pull/11144#discussion_r4506183280
##########
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:
The values decoder can't see the Arrow type.
`GenericRecordReader::set_page_reader` creates a new decoder with
`CV::new(&column_desc)` for every column chunk, and that constructor only gets
the column descriptor. So the flag has to live on something that outlives each
decoder, and the array reader only holds the record reader. I added a sentence
to the setter's doc saying so.
The alternatives I could see:
- Validate in the array readers after decoding, where the Arrow type is
known. That works for `Utf8`, `LargeUtf8` and `Utf8View`. But dictionary values
are decoded once per dictionary page and shared between batches, so they would
need checking again in every batch, or caching.
- Pass the Arrow type to `ColumnValueDecoder::new`, which changes every
decoder.
The field on the record reader seemed the smallest of the three, but I'm
happy to switch if you prefer one of the others.
##########
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:
Yes, that's the whole fix for this reader. The view and dictionary readers
make the same check for their own types.
##########
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:
`|=` means this can only ever turn validation on. The record reader passes
its flag to every new decoder, and the flag is `false` by default. With `=`, a
reader that never called `set_validate_utf8` would switch validation off for a
column annotated as a string.
Today `=` would behave the same, because a column annotated as a string can
only be read as a string type, so all three readers pass `true` for it. I kept
`|=` as the safer choice, but can change it if you'd rather have `=`.
--
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]
