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
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 |
Which issue does this PR close?
Closes #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.