Skip to content

fix: Normalize s3 paths for PME key retriever - #2874

Merged
mbutrovich merged 2 commits into
apache:mainfrom
mbutrovich:normalize_s3_pme
Dec 11, 2025
Merged

mbutrovich merged 2 commits into
apache:mainfrom
mbutrovich:normalize_s3_pme

Conversation

@mbutrovich

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #2871.

Rationale for this change

What changes are included in this PR?

Normalize s3 paths in the key retriever cache.

How are these changes tested?

Added a test that fails on main native_datafusion Parquet reader. With this change, the test passes.

@codecov-commenter

codecov-commenter commented Dec 10, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.28%. Comparing base (f09f8af) to head (e226bb6).
⚠️ Report is 751 commits behind head on main.

Files with missing lines Patch % Lines
...rg/apache/comet/parquet/CometFileKeyUnwrapper.java 0.00% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2874      +/-   ##
============================================
+ Coverage     56.12%   59.28%   +3.15%     
- Complexity      976     1345     +369     
============================================
  Files           119      167      +48     
  Lines         11743    15306    +3563     
  Branches       2251     2536     +285     
============================================
+ Hits           6591     9074    +2483     
- Misses         4012     4944     +932     
- Partials       1140     1288     +148     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread common/src/main/java/org/apache/comet/parquet/CometFileKeyUnwrapper.java Outdated
…apper.java

Co-authored-by: Andy Grove <agrove@apache.org>
@mbutrovich
mbutrovich requested a review from andygrove December 11, 2025 00:04
assert(df.first().getLong(0) == 499500)
}

test("write and read encrypted parquet from S3") {

@andygrove andygrove Dec 11, 2025 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know if this test exercises the changes to normalizing the path or not, but the test looks good

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It did fail on main, FWIW.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @mbutrovich

@mbutrovich
mbutrovich merged commit 58060e2 into apache:main Dec 11, 2025
113 checks passed
@mbutrovich
mbutrovich deleted the normalize_s3_pme branch December 11, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parquet Encryption fails due to path mismatch (s3a:// vs s3://) in CometFileKeyUnwrapper cache

3 participants