fix(parquet): honor explicit V1 FLBA dictionary opt-in - #10569
Conversation
Preserve the existing Parquet 1.0 default for fixed-length byte arrays, but honor explicit global and per-column dictionary settings. Update writer and CDC coverage for the newly reachable dictionary path.
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @subotac, nice job keeping the old behavior.
I do feel somewhat conflicted about this. I think the decision to not support FLBA dict encoding for 1.0 was a mistake from the get-go. Clearly by 2014 even parquet-mr was able to read this combination, so I highly doubt there are any readers still in use that cannot (as evidence arrow-cpp has no such restriction on using dict encoding with FLBA). If we drop maintaining the old behavior this PR can be made much simpler. I'll wait a bit and let others opine on the best way forward here before merging.
|
is this something we should raise as a question to parquet and/or parquet-java (formerly parquet-mr)? it does seem that this decision stemmed from parquet-mr (as linked in the issue thread, apache/parquet-java#30) and the implementation here followed that (sunchao/parquet-rs#196) i was trying to check the parquet format at version 1.0.0 and was quite confused as my read was nothing prevents fixed len byte arrays from being dictionary encoded either 🤔 |
I think that's probably a good idea, given that I cannot find this behavior documented anywhere in the parquet-java API docs nor the spec. In the meantime, I think this PR does a nice job of threading the needle...it maintains the current behavior by default, but allows for users to explicitly enable dict encoding. Before merging, I think the docs for the writer properties should be updated to explain the current state (i.e. by default, dictionary encoding will not be used if the writer version is 1.0, but users can explicitly enable dictionary encoding either globally or per column to override this default behavior). @subotac, would you be willing to add this documentation? |
|
I updated both dictionary-setting methods to document this explicitly: V1 FLBA remains default-off, while an explicit global or per-column |
|
i'm kinda of the mind to just enable it by default, instead of honoring the previous default behaviour. for reference, was able to use codex to generate some duckdb code which shows that they can generate such files so i'm fairly sure its only a parquet-java limitation: DuckDB v1.5.5 (Variegata)
Enter ".help" for usage hints.
memory D CREATE TABLE values_to_write AS
SELECT
CASE
WHEN i % 2 = 0
THEN UUID '00000000-0000-0000-0000-000000000001'
ELSE UUID '00000000-0000-0000-0000-000000000002'
END AS value
FROM range(200) AS rows(i);
memory D
memory D COPY values_to_write
TO 'duckdb-v1-flba-dictionary.parquet'
(
FORMAT PARQUET,
PARQUET_VERSION V1,
COMPRESSION UNCOMPRESSED
);
memory D SELECT value, count(*)
FROM read_parquet('duckdb-v1-flba-dictionary.parquet')
GROUP BY value
ORDER BY value;
┌──────────────────────────────────────┬──────────────┐
│ value │ count_star() │
│ uuid │ int64 │
├──────────────────────────────────────┼──────────────┤
│ 00000000-0000-0000-0000-000000000001 │ 100 │
│ 00000000-0000-0000-0000-000000000002 │ 100 │
└──────────────────────────────────────┴──────────────┘
memory D SELECT
path_in_schema,
type,
encodings,
dictionary_page_offset
FROM parquet_metadata('duckdb-v1-flba-dictionary.parquet');
┌────────────────┬──────────────────────┬──────────────────┬────────────────────────┐
│ path_in_schema │ type │ encodings │ dictionary_page_offset │
│ varchar │ varchar │ varchar │ int64 │
├────────────────┼──────────────────────┼──────────────────┼────────────────────────┤
│ value │ FIXED_LEN_BYTE_ARRAY │ PLAIN_DICTIONARY │ 4 │
└────────────────┴──────────────────────┴──────────────────┴────────────────────────┘
memory D SELECT format_version
FROM parquet_file_metadata('duckdb-v1-flba-dictionary.parquet');
┌────────────────┐
│ format_version │
│ int64 │
├────────────────┤
│ 1 │
└────────────────┘
memory Dthe issue mentions
maybe if we double check against parquet-java, to see if it can read such files, we can just simplify this PR |
Yes, it seems to be an ancient parquet-mr holdover. I'm pretty sure java has been able to read this for quite some time.
I used pyarrow to generate a file with an FLBA column dict encoded and parquet-java 1.14 reads it just fine. I'd prefer to align with parquet-cpp and just enable the combination without opt-in. But I'm fine with the current state of this PR as well. |
@subotac are you able to implement @etseidl 's request to avoid the opt in ? This PR also appears to have some merge conflicts that need to be resolved before we can merge it |
|
Merged current main to resolve the conflicts. \cargo test -p parquet --lib\ passes locally (1,380 tests). @etseidl, could you take another look once the checks finish? |
| // 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) | ||
| } |
There was a problem hiding this comment.
| // 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) | |
| } |
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.
|
|
||
| #[test] | ||
| fn test_column_writer_default_encoding_support_fixed_len_byte_array() { | ||
| let default_props = WriterProperties::builder() |
There was a problem hiding this 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.
4 participants
Which issue does this PR close?
set_dictionary_enabled(true)is silently ignored for FIXED_LEN_BYTE_ARRAY columns at the default writer version #10524.Rationale for this change
Parquet 1.0 writers silently ignored explicit dictionary settings for
FIXED_LEN_BYTE_ARRAYcolumns, even though dictionary encoding is valid for the physical type.What changes are included in this PR?
Honor explicit global and per-column dictionary opt-ins for Parquet 1.0 FLBA columns while preserving the existing default-off behavior. Update CDC expectations for FixedSizeBinary arrays when dictionary encoding is
explicitly enabled.
Are these changes tested?
Yes. Added coverage for the default behavior, global opt-in, per-column override, and explicit disable cases. The focused tests, full Parquet suite, Clippy, and formatting checks pass.
Are there any user-facing changes?
Callers can now explicitly enable dictionary encoding for Parquet 1.0 FLBA columns. The default behavior is unchanged.