-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Implement CatalogProviderList in FFI #18657
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
Implement CatalogProviderList in FFI #18657
Conversation
martin-g
left a 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.
Few more naming suggestions
|
@martin-g Thank you for the review! Most of those are copy+paste things I didn't catch. I didn't change the recommendations where you were suggesting something that differs from the underlying trait. Maybe we should update the naming there, but honestly feels a bit pedantic. I think the function names here should mirror those in the underlying trait. |
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 -- thanks @timsaucer and @martin-g
I agree |
Which issue does this PR close?
None
Rationale for this change
This is continuation of work in the FFI crate to expose useful traits. We currently have FFI catalog, schema, and table providers. The next layer up in the heirarchy is the catalog provider list.
What changes are included in this PR?
Are these changes tested?
Yes, included in PR.
Are there any user-facing changes?
This is only new addition to the FFI crate. No existing code is modified except making one data member pub(crate)