etseidl commented on code in PR #11266:
URL: https://github.com/apache/arrow-rs/pull/11266#discussion_r4127162535
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -148,6 +148,10 @@ impl FallbackEncoder {
WriterVersion::PARQUET_2_0 => Encoding::DELTA_BYTE_ARRAY,
});
+ crate::encodings::encoding::validate_column_encoding(
Review Comment:
calling this is duplicating the default arm of the following match. Other
than standardizing messages I don't know that this is strictly necessary. And
it means if we add a new byte array encoding, we have to modify this check in
two places.
If we keep it anyway, let's no fully qualify it.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -175,6 +176,52 @@ pub(crate) mod private {
}
}
+fn unsupported_column_encoding(encoding: Encoding, physical_type: Type) ->
ParquetError {
+ if encoding == Encoding::ALP {
Review Comment:
why is ALP the only special case here. for instance, BIT_PACKED is not
supported at all, but now the error message will be it isn't supported for type
X, which sort of implies it _is_ supported for some other type.
Since this is only called right before entering a match on encoding, why
don't we just shore up the type checking in those matches, so there's a single
place where this stuff is encoded/enforced.
##########
parquet/src/encodings/encoding/mod.rs:
##########
@@ -849,23 +896,39 @@ mod tests {
// supported encodings
create_and_check_encoder::<Int32Type>(0, Encoding::PLAIN, None);
create_and_check_encoder::<Int32Type>(0,
Encoding::DELTA_BINARY_PACKED, None);
- create_and_check_encoder::<Int32Type>(0,
Encoding::DELTA_LENGTH_BYTE_ARRAY, None);
- create_and_check_encoder::<Int32Type>(0, Encoding::DELTA_BYTE_ARRAY,
None);
Review Comment:
I see your point though...why did this ever pass?
--
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]
