-
Notifications
You must be signed in to change notification settings - Fork 175
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
[FEAT] Infer Azure storage account from uri #3165
Conversation
@anilmenon14 could you check if the changes in this PR works? Let me know if you need any help with that |
CodSpeed Performance ReportMerging #3165 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3165 +/- ##
==========================================
- Coverage 79.00% 78.99% -0.02%
==========================================
Files 634 634
Lines 76943 76958 +15
==========================================
+ Hits 60789 60790 +1
- Misses 16154 16168 +14
|
Hi @kevinzwang , the changes look great to me! I did a quick build of this branch on my laptop and ran some tests and they passed.
This worked perfectly,by materializing the expected results of the Dataframe! 🙌 I also did a quick regression test using an AWS environment to confirm there has been no unintended impact seen elsewhere and it still works well there too. |
@anilmenon14 thanks for testing it! One final thing I'd like to confirm is that with your setup, the script fails without these changes, AKA that the Azure storage account was not set elsewhere in your testing, and that it is properly determined by Daft with this PR. |
Hi @kevinzwang , your statements are correct. With the code in this PR, run on a shell session without any pre-set environment variables for the Azure storage account, it is now able to determine the storage account. |
That's great to hear! Thank you again for such thorough testing. @raunakab please give this a review whenever you can |
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.
Looks good to me!
Resolves #3142
This PR also adds a dependency for the
derive_builder
crate, which I'm hoping to introduce into our repo and use throughout our code to reduce builder boilerplate.