Skip to content

[58_maintenance] [parquet] Allow more encryption algorithms (#9203)#10351

Open
mbutrovich wants to merge 1 commit into
apache:58_maintenancefrom
mbutrovich:58_maintenance_9203
Open

[58_maintenance] [parquet] Allow more encryption algorithms (#9203)#10351
mbutrovich wants to merge 1 commit into
apache:58_maintenancefrom
mbutrovich:58_maintenance_9203

Conversation

@mbutrovich

@mbutrovich mbutrovich commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

@hsiang-c @mbutrovich
[parquet] Allow more encryption algorithms (apache#9203)
98f3911 
# Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax.
-->

- Closes apache#9202.

# Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->

- Iceberg
[spec](https://iceberg.apache.org/gcm-stream-spec/#encryption-algorithm)
supports AES key sizes of 128, 192 and 256 bits. Iceberg Rust depends on
`arrow-rs` for Parquet I/O, I'd like to start supporting AES 256 with
this PR.

# What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
- `RingGcmBlockEncryptor` and `RingGcmBlockDecryptor` will pick AES-128
or AES-256 based on key size
- Refactor `encryption_async.rs` and `encryption.rs` to test both
AES-128 and AES-256 encrypted parquet files

# Are these changes tested?

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->

Yes, unit test and on AES-256 encrypted Parquet files defined in
https://github.com/apache/parquet-testing/tree/master/data/aes256

# Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.

If there are any breaking changes to public APIs, please call them out.
-->

No

(cherry picked from commit accb1cf)
@github-actions github-actions Bot added the parquet Changes to the parquet crate label Jul 15, 2026
@mbutrovich mbutrovich mentioned this pull request Jul 15, 2026
Open
4 tasks
@mbutrovich

mbutrovich commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Hm, what's the best way to tackle the MSRV issue?

hsiang-c
hsiang-c approved these changes Jul 15, 2026
View reviewed changes

@hsiang-c hsiang-c 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.

LGTM

@mbutrovich

mbutrovich commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Testing a change that adds --locked to the MSRV check workflow. I think this is reasonable for 58_maintenance branch. We can discuss if that makes sense to do in main as well.

@mbutrovich

mbutrovich commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Testing a change that adds --locked to the MSRV check workflow. I think this is reasonable for 58_maintenance branch. We can discuss if that makes sense to do in main as well.

That made CI worse, I'll take a look in the AM.

@mbutrovich
mbutrovich force-pushed the 58_maintenance_9203 branch from f5aa920 to 98f3911 Compare July 16, 2026 13:50
@mbutrovich

mbutrovich commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

#7290 somewhat fixed this in main. Seeing if that cherry-pick fixes it.

@mbutrovich

mbutrovich commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

The failing MSRV check is not specific to this PR (which only touches parquet/, the parquet-testing submodule, and the MSRV workflow).

Cause: no Cargo.lock is committed, so cargo msrv verify resolves fresh each run. With resolver = "2" (not rust-version-aware) it selects the newest dep versions, which now exceed MSRV 1.85 -- e.g. tonic 0.14.6 requires 1.88. The --locked change on this branch surfaced it by getting the job past cargo install cargo-msrv.

main reproduces it. cargo msrv verify on main's HEAD tree, 1.85 toolchain:

error: rustc 1.85.0 is not supported by the following packages:
  tonic@0.14.6 requires rustc 1.88

main's CI is currently green, which appears to be a cached registry index (predating tonic 0.14.6); a cold resolve fails.

Two fixes verified locally (cargo msrv verify, 1.85):

  • resolver = "3" (edition 2024 default): is_compatible: true -- back-selects tonic 0.14.5, icu 2.1.x. Covers any dep declaring a rust-version.
  • Manual cargo update -p <crate> --precise pins in the MSRV job (as in Fix MSRV CI Check (pin half dependency) #7290): also works; one entry per offender.

Questions for @alamb:

  1. Preference between the two approaches above?
  2. Land on main first (since it has the same latent failure), then backport to 58_maintenance?

The C Data Interface (Java to Rust) failure appears unrelated (Java allocator leak).

@alamb

alamb commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Manual cargo update -p --precise pins in the MSRV job (as in #7290): also works; one entry per offender.

I think we should do this.

@mbutrovich mbutrovich mentioned this pull request Jul 17, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@hsiang-c hsiang-c hsiang-c approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

parquet Changes to the parquet crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mbutrovich @alamb @hsiang-c