alamb commented on code in PR #16214:
URL: https://github.com/apache/datafusion/pull/16214#discussion_r2115790804
##########
datafusion/core/tests/integration_tests/schema_adapter_integration_tests.rs:
##########
@@ -258,3 +259,187 @@ fn test_schema_adapter_preservation() {
// Verify the schema adapter factory is present in the file source
assert!(config.source().schema_adapter_factory().is_some());
}
+
Review Comment:
this test is moved without change from
`datafusion/core/tests/test_adapter_updated.rs`
##########
datafusion/datasource/src/file.rs:
##########
@@ -126,19 +126,26 @@ pub trait FileSource: Send + Sync {
/// Set optional schema adapter factory.
///
/// [`SchemaAdapterFactory`] allows user to specify how fields from the
- /// file get mapped to that of the table schema. The default implementation
- /// returns the original source.
+ /// file get mapped to that of the table schema. If you implement this
+ /// method, you should also implement [`schema_adapter_factory`].
///
- /// Note: You can implement this method and `schema_adapter_factory`
- /// automatically using the [`crate::impl_schema_adapter_methods`] macro.
+ /// The default implementation returns a not implemented error.
+ ///
+ /// [`schema_adapter_factory`]: Self::schema_adapter_factory
Review 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
##########
datafusion/core/src/datasource/physical_plan/arrow_file.rs:
##########
@@ -99,7 +99,19 @@ impl FileSource for ArrowSource {
"arrow"
}
- impl_schema_adapter_methods!();
+ fn with_schema_adapter_factory(
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
