Skip to content

fix(parquet): honor explicit V1 FLBA dictionary opt-in - #10569

Open
subotac wants to merge 3 commits into
apache:mainfrom
subotac:fix/parquet-v1-flba-dictionary
Open

fix(parquet): honor explicit V1 FLBA dictionary opt-in#10569
subotac wants to merge 3 commits into
apache:mainfrom
subotac:fix/parquet-v1-flba-dictionary

Conversation

@subotac

@subotac subotac commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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_ARRAY columns, 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.

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.
@github-actions github-actions Bot added the parquet Changes to the parquet crate label Aug 6, 2026

@etseidl etseidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cc @alamb @Jefffrey @jhorstmann

@Jefffrey Jefffrey added the bug label Aug 7, 2026
@Jefffrey

Jefffrey commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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 🤔

@etseidl

etseidl commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

is this something we should raise as a question to parquet and/or parquet-java (formerly parquet-mr)?

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?

@subotac

subotac commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

I updated both dictionary-setting methods to document this explicitly: V1 FLBA remains default-off, while an explicit global or per-column true enables dictionary encoding.

@subotac
subotac requested a review from etseidl August 7, 2026 21:12

@etseidl etseidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @subotac, this looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[parquet] set_dictionary_enabled(true) is silently ignored for FIXED_LEN_BYTE_ARRAY columns at the default writer version

3 participants