refactor: return Result from fallible catalog and schema methods#21944
refactor: return Result from fallible catalog and schema methods#21944gratus00 wants to merge 4 commits into
Conversation
…e better replaced with ? or match
| .expect("catalog listed by context should be retrievable"); | ||
| for schema in catalog_provider | ||
| .schema_names() | ||
| .expect("schema names lookup should succeed") |
There was a problem hiding this 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.
| let foreign_catalog: Arc<dyn CatalogProvider> = (&ffi_catalog).into(); | ||
|
|
||
| let prior_schema_names = foreign_catalog.schema_names(); | ||
| let prior_schema_names = foreign_catalog |
There was a problem hiding this 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 DataFusionErrors.
That would give this PR's main contract some coverage at the FFI boundary too.
|
Hi @kosiew, thank you for your review! I'll take a look at this hopefully next week if there doesn't seem to be a rush! |
|
@gratus00 |
| fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>> { | ||
| fn schema(&self, name: &str) -> Result<Option<Arc<dyn SchemaProvider>>> { | ||
| let schemas = self.schemas.read().unwrap(); | ||
| let maybe_schema = schemas.get(name); |
There was a problem hiding this comment.
Small cleanup suggestion: this manual if let Some can be collapsed to the direct lookup result, since the map already stores Arc<dyn SchemaProvider>:
Ok(schemas.get(name).cloned())|
I also added the api change to this PR |
Which issue does this PR close?
Rationale for this change
Several
CatalogProviderandSchemaProvidermethods are fallible in practice, but their signatures do not currently returnResult. That forces implementations and callers to either hide errors or treat fallible operations as infallible.This change makes those fallible paths explicit so errors can be propagated correctly through the catalog API and its downstream callers.
What changes are included in this PR?
This PR updates the following methods to return
Result:CatalogProvider::schema_namesCatalogProvider::schemaSchemaProvider::table_namesSchemaProvider::table_existIt also propagates those signature changes through the relevant implementations and call sites, including:
Are these changes tested?
Yes.
The following commands were run locally:
Are there any user-facing changes?
There are no end-user SQL or CLI behavior changes.
This is a public API change for implementers of CatalogProvider and SchemaProvider, so the api change label should be added.