kosiew commented on code in PR #21944:
URL: https://github.com/apache/datafusion/pull/21944#discussion_r3217967915
##########
datafusion/ffi/src/catalog_provider.rs:
##########
@@ -382,7 +384,9 @@ mod tests {
let foreign_catalog: Arc<dyn CatalogProvider> = (&ffi_catalog).into();
- let prior_schema_names = foreign_catalog.schema_names();
+ let prior_schema_names = foreign_catalog
Review Comment:
The FFI round-trip tests cover the successful path after the ABI changed to
`FFI_Result`, which is great. It would also be helpful to have a small failing
provider test that asserts failures from `schema_names` and `schema` cross the
FFI boundary as `DataFusionError`s.
That would give this PR's main contract some coverage at the FFI boundary
too.
##########
datafusion-examples/examples/flight/sql_server.rs:
##########
@@ -176,12 +176,26 @@ impl FlightSqlServiceImpl {
let mut names = vec![];
let mut types = vec![];
for catalog in ctx.catalog_names() {
- let catalog_provider = ctx.catalog(&catalog).unwrap();
- for schema in catalog_provider.schema_names() {
- let schema_provider =
catalog_provider.schema(&schema).unwrap();
- for table in schema_provider.table_names() {
- let table_provider =
- schema_provider.table(&table).await.unwrap().unwrap();
+ let catalog_provider = ctx
+ .catalog(&catalog)
+ .expect("catalog listed by context should be retrievable");
+ for schema in catalog_provider
+ .schema_names()
+ .expect("schema names lookup should succeed")
Review Comment:
Nice update here. One small follow-up: this example still turns the newly
fallible catalog, schema, and table name lookups into panics via `expect`.
Since this endpoint already returns `Result<_, Status>`, it might be worth
changing `tables` to return `Result<RecordBatch, Status>` and mapping these
errors to `Status::internal(...)` with `?`. That would keep the example aligned
with the PR's invariant that catalog failures propagate to callers instead of
aborting the server.
--
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]
