-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AMORO-2401] Support reading encrypted iceberg data files #2402
Conversation
@liaoyt Thanks a lot for your contribution! You may need to sign NetEase's CLA before you start to contribute to Amoro, you can conveniently complete the signing via the link: https://cla-assistant.io/NetEase/amoro |
thanks for reminding, already signed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2402 +/- ##
============================================
- Coverage 53.52% 53.51% -0.01%
+ Complexity 4371 4368 -3
============================================
Files 517 517
Lines 29900 29912 +12
Branches 2917 2917
============================================
+ Hits 16003 16008 +5
- Misses 12608 12613 +5
- Partials 1289 1291 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@liaoyt Thanks for your contribution,I want to know if you are currently using the encryption feature. |
yes, although iceberg has only |
I am very interested in this area. How does your EncryptionManager work? Currently, the implementation of BaseTable does not provide a custom method. Did you wrap your own Table object? |
There is not custom method provided currently, we just overwrite the |
As for the behavior of reading data file of iceberg table, iceberg already implement it. This PR attempt to keep this behavior the same as iceberg do. |
I got you, BTW, GenericDeleteFilter also need EncryptionManager |
thanks for reminding, i'll have a look and fix it later, maybe tomorrow. |
Can you add some unit tests to verify if it's working? |
sure,i can try add some unit test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for your contributions |
Why are the changes needed?
Close #2401.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation