alamb commented on code in PR #16148:
URL: https://github.com/apache/datafusion/pull/16148#discussion_r2109906320
##########
datafusion/core/tests/test_adapter_updated.rs:
##########
@@ -0,0 +1,214 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Due to
1. THe previously mentioned reasons that more targets results in longer
compile times
2. It will be harder to find tests for the same feature if they are in
different files
I think we should remove this new file
`datafusion/core/tests/test_adapter_updated.rs` and put the test in
`datafusion/core/tests/integration_tests/schema_adapter_integration_tests.rs`
instead
However, we can do that as a follow on PR too
##########
datafusion/datasource/src/macros.rs:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Macros for the datafusion-datasource crate
+
+/// Helper macro to generate schema adapter methods for FileSource
implementations
Review Comment:
this is super well documented. I am still not a huge fan of adding this new
macro as I think it makes implementing DataSources that much more complicated,
but I can see the rationale for not adding a default on
https://github.com/apache/datafusion/pull/16148#discussion_r2106508912
So let's go with this approach and see how it goes
##########
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:
> It forces every callsite—even the 99% that never “fail”—to handle a
Result. That’s a lot of boilerplate up front.
IN my opinion the boilerplate would be realtively minimal (likely it would
require `?` a few places)
> Pushing the “not implemented” case to runtime means we only discover
missing overrides via panics or errors in production, instead of compile-time
feedback.
This argument makes sense to me. However, another drawback is that after
this PR, it is required for `DataSource` implementations to handle schema
adapter, rather than allowing an implementation to return a runtime error if it
doesn't
I think that is probably fine but in the future I can still see a usecase
for a fallable Result
--
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
