kosiew commented on code in PR #16148:
URL: https://github.com/apache/datafusion/pull/16148#discussion_r2106508912
##########
datafusion/datasource/src/test_util.rs:
##########
@@ -81,6 +83,8 @@ impl FileSource for MockSource {
fn file_type(&self) -> &str {
"mock"
}
+
+ impl_schema_adapter_methods!();
Review Comment:
Hey, really appreciate the suggestion! Turning these two methods into trait
defaults is
[tempting](https://github.com/apache/datafusion/pull/16148/commits/ee07b69fbcc0a41bd2a859f5f0fa328b35d4e69d),
but we run into some frustrating object-safety and cloning issues:
1. Fallible signature (Result)
Switching to
```rust
fn with_schema_adapter_factory(...) -> Result<Arc<dyn FileSource>, _>
```
also has drawbacks:
It forces every callsite—even the 99% that never “fail”—to handle a Result.
That’s a lot of boilerplate up front.
Pushing the “not implemented” case to runtime means we only discover missing
overrides via panics or errors in production, instead of compile-time feedback.
2. Trait-object vs. Clone
A default like
```rust
fn with_schema_adapter_factory(
&self,
_factory: Arc<dyn SchemaAdapterFactory>,
) -> Arc<dyn FileSource> {
Arc::new(self.clone())
}
```
can’t compile because:
self is a &Self, so self.clone() gives you another &Self, producing an
Arc<&Self>, and &Self isn’t a FileSource.
To make it work you’d need Self: Sized + Clone on the default—but then that
method isn’t even available on dyn FileSource, defeating trait-object use.
So, I am leaning towards keeping the macro because:
- Object-safe clone: by generating Arc::new(Self { …, ..self.clone() })
inside each impl, the macro leverages the concrete type’s Clone impl without
polluting the trait itself.
- Single maintenance point: if we ever tweak the method signature, we update
the macro once and every impl site gets fixed automatically.
- Compile-time assurance: missing impl_schema_adapter_methods!() on a type
immediately fails to compile, alerting the author they need to opt in.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
