Skip to content

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

Merged
alamb merged 2 commits into
apache:58_maintenancefrom
mbutrovich:58_maintenance_9203
Jul 17, 2026
Merged

[58_maintenance] [parquet] Allow more encryption algorithms (#9203)#10351
alamb merged 2 commits 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
Merged
alamb pushed a commit that referenced this pull request Jul 17, 2026
@mbutrovich
[58_maintenance] Fix MSRV CI check (pin tonic to 0.14.5, install carg…
adb77a1 
…o-msrv --locked) (#10365)

# Which issue does this PR close?

<!-- No dedicated issue for the CI break; file one if a tracker is
wanted. -->

- Part of #10349 (58.4.0 release effort).
- Unblocks #10351, whose CI surfaced this pre-existing failure.
- Prior art: #7290 (Fix MSRV CI Check), same class of fix.

# Rationale for this change

The `Verify MSRV` job on `58_maintenance` is failing on every PR
(surfaced by #10351, whose own changes are parquet-only and unrelated).
Two independent, pre-existing problems, both from dependency drift:

1. **`cargo install cargo-msrv` fails to build.** Installed unlocked,
cargo-msrv's transitive deps resolve to their newest versions; recent
`aws-*` releases require rustc 1.94.1, newer than the CI container's
rustc, so the install fails before `verify` runs.

2. **`cargo msrv verify` fails on `arrow-flight`.** We commit no
`Cargo.lock`, so `verify` resolves fresh each run and picks the newest
deps. `tonic 0.14.6` now requires rustc 1.88, above our 1.85 MSRV.

# What changes are included in this PR?

Both changes are in `.github/workflows/rust.yml` (MSRV job only):

- Install cargo-msrv with `--locked`, so its transitive deps resolve to
versions that build on the CI container's rustc instead of the newest
published ones.
- Add a `Downgrade workspace dependencies` step that pins the tonic
crates to 0.14.5 (the latest release supporting rustc 1.85) before
`cargo msrv verify`, following the approach in #7290. The tonic crates
are downgraded in matched pairs (`tonic`/`tonic-prost`, then
`tonic-build`/`tonic-prost-build`) so their inter-crate `^` requirements
stay satisfiable.

No source, `Cargo.toml`, or declared-MSRV changes.

# Are these changes tested?

Yes, verified locally against a 1.85.1 toolchain with `cargo-msrv`
0.19.3 (matching CI):

- Without the pins: `cargo msrv verify` on `arrow-flight` reports
`is_compatible: false` (`tonic 0.14.6` requires 1.88).
- With the pins: the full CI `find` loop (`cargo msrv verify` over all
29 packages) passes.

# Are there any user-facing changes?

No. CI-only change; no public API, code, or declared-MSRV changes.

---

**Note for reviewers:** `main` has the same latent failure -- I ran
`cargo msrv verify` on `main`'s HEAD and it fails identically on `tonic
0.14.6`. Its CI is currently green only because of a cached registry
index predating that release. The `--locked` fix is already on `main`;
the tonic pin step is not. Suggest a follow-up applying the tonic pin
step to `main` as well.
@alamb

alamb commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Merged up to hopefully get fix for

alamb
alamb approved these changes Jul 17, 2026
View reviewed changes

@alamb alamb 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.

Looks good -- thanks @mbutrovich and @hsiang-c and @Jefffrey (indirectly!)

I will now prepare a release candidate

@alamb
alamb merged commit 01046ee into apache:58_maintenance Jul 17, 2026
16 checks passed
@alamb alamb mentioned this pull request Jul 17, 2026
Open
@mbutrovich
mbutrovich deleted the 58_maintenance_9203 branch July 17, 2026 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes
+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