Simplify FileSource / SchemaAdapterFactory API #16214
Conversation
github-actions
bot
added
core
| } | ||
|
|
||
| impl_schema_adapter_methods!(); | ||
| fn with_schema_adapter_factory( |
There was a problem hiding this comment.
instead of the impl_schema_adapter_methods macro, I instead replicated the code several places. While there is more duplicated code I think it is clearer because what is happening is now more explicit and doesn't require a macro (even though impl_schema_adapter_methods was super well documented)
| // Verify the schema adapter factory is present in the file source | ||
| assert!(config.source().schema_adapter_factory().is_some()); | ||
| } | ||
|
|
There was a problem hiding this comment.
this test is moved without change from datafusion/core/tests/test_adapter_updated.rs
| /// automatically using the [`crate::impl_schema_adapter_methods`] macro. | ||
| /// The default implementation returns a not implemented error. | ||
| /// | ||
| /// [`schema_adapter_factory`]: Self::schema_adapter_factory |
There was a problem hiding this comment.
this signature is changed to return a Result and defaults to not implemented
As @kosiew points out this means that implementors of a FileSource will only get an error at runtime, not compile time, but I think that is better than forcing everyone to implement a schema adapter even if they don't need it
|
@xudong963 would you have a moment to help review this PR? It is something I would like to get in before 48.0.0 is released (as it contains a change to an as yet unreleased API) |
|
Thank you @xudong963 |
* Make FileFormat::with_schema_adapter_factory fallible, remove macros * Remove standalone integration test * Update doc test
3 participants
Which issue does this PR close?
Rationale for this change
During the review of #16148 from @kosiew 🙏 @adriangb and I had some suggestions to simplify the API and make upgrading easier. However, it wasn't clear how they would look so I tried them out and it turns out they worked well
What changes are included in this PR?
This PR simplifies the API:
FileForma::with_schema_adapter_factoryto return an ResultFileFormatimplementations)impl_schema_adapter_methodsmacroAre these changes tested?
Yes by CI
Are there any user-facing changes?
Since none of the modified APIs have been released, this is not a breaking change.