etseidl commented on code in PR #10569:
URL: https://github.com/apache/arrow-rs/pull/10569#discussion_r4007180824
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1978,13 +1978,14 @@ fn fallback_encoding(kind: Type, props:
&WriterProperties) -> Encoding {
}
/// Returns true if dictionary is supported for column writer, false otherwise.
-fn has_dictionary_support(kind: Type, props: &WriterProperties) -> bool {
+fn has_dictionary_support(kind: Type, props: &WriterProperties, path:
&ColumnPath) -> bool {
match (kind, props.writer_version()) {
// Booleans do not support dict encoding and should use a fallback
encoding.
(Type::BOOLEAN, _) => false,
- // Dictionary encoding was not enabled in PARQUET 1.0
- (Type::FIXED_LEN_BYTE_ARRAY, WriterVersion::PARQUET_1_0) => false,
- (Type::FIXED_LEN_BYTE_ARRAY, WriterVersion::PARQUET_2_0) => true,
+ // Preserve the PARQUET_1_0 default, but honor an explicit opt-in.
+ (Type::FIXED_LEN_BYTE_ARRAY, WriterVersion::PARQUET_1_0) => {
+ props.dictionary_enabled_setting(path) == Some(true)
+ }
Review Comment:
```suggestion
```
Thanks @subotac, but I think what @Jefffrey was suggesting (and I now
concur) is to remove this behavior altogether. In other words, users would need
to opt out of dictionary encoding explicitly, regardless of the parquet writer
version.
##########
parquet/src/column/writer/mod.rs:
##########
@@ -2571,14 +2572,36 @@ mod tests {
#[test]
fn test_column_writer_default_encoding_support_fixed_len_byte_array() {
+ let default_props = WriterProperties::builder()
Review Comment:
Then this test would just check that for both v1 and v2 the writer uses dict
encoding. Or we could just remove this test since FLBA is no longer a special
case.
--
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]
