-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(python): Parse uppercase config keys #19852
fix(python): Parse uppercase config keys #19852
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #19852 +/- ##
==========================================
+ Coverage 79.33% 79.46% +0.13%
==========================================
Files 1548 1555 +7
Lines 214247 216315 +2068
Branches 2460 2456 -4
==========================================
+ Hits 169971 171905 +1934
- Misses 43718 43852 +134
Partials 558 558 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Thanks @ion-elgreco. Can you add a test as well? |
@ritchie46 sure, Ill take a look Sunday. Are there existing integration tests with any object storage? |
I think implicitly in python by forcing async. |
We can also add a simple rust unit test with a comment on why that is tested. E.g. test parsing an uppercase config. |
Sure will do that, don't have time until Sunday for it tho |
.map_err( | ||
|_| polars_err!(ComputeError: "unknown configuration key: {}", key.as_ref()), | ||
) | ||
.filter_map(|(key, val)| { |
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.
Now invalid keys are silently ignored? That doesn't seem right.
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.
I am doing it explicitly. In delta-rs for example we use quite some custom storage_options for our internal log/objectstore handling.
So when you do a read_delta and you pass those additional options from your DeltaTable object into pola-rs, it will error out, see this comment #19103 (comment)
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.
Right, can you add a comment that some keys are read upstream and may silently be ignored there.
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.
Donee
Thank you @ion-elgreco. |
In delta-rs we also parse config keys that are uppercase, this provides more flexibility and allows people to use same storage options for delta-rs and pola-rs.
Additionally instead of raising for non existing config keys, we just filter them out. This would make it easier to keep engine specific keys in the storage options in python, since delta-rs has some of those to handle mounted storage.