refactor: Change SchemaProvider::table to return Result<Option<..> rather than Option<..>

[crepererum]

Which issue does this PR close?

Closes #9305.

Rationale for this change

SchemaProvider::table is async and clearly allows IO:

https://github.com/apache/arrow-datafusion/blob/6fad5ed7a37c50b9c200f214c3e13b0e1f0cecbc/datafusion/core/src/catalog/schema.rs#L114-L116

However this method cannot fail. Casting errors to None is semantically wrong.

What changes are included in this PR?

Change return type from Option<Arc<dyn TableProvider>> to Result<Option<Arc<dyn TableProvider>>, DataFusionError>.

Are these changes tested?

It compiles.

Are there any user-facing changes?

Breaking: Interface change.

[comphead]

lgtm thanks @crepererum
please address the minor.

Basically I'm thinking to create a type alias for Result<Option> as such nested generics are hard to read

[crepererum]

Basically I'm thinking to create a type alias for Result as such nested generics are hard to read

I agree on the readability aspects, but I also see the following counter points:

1. The name of that alias is likely not shorter
2. Naming a "result of an option" is hard, esp. since it should make sure it's not an "optional result". To my knowledge there is no ecosystem-wide wording for it
3. Any alias is an indirection that the reader has to map mentally, while w/o it you can instantly see the type

[comphead]

Basically I'm thinking to create a type alias for Result as such nested generics are hard to read

I agree on the readability aspects, but I also see the following counter points:

1. The name of that alias is likely not shorter
2. Naming a "result of an option" is hard, esp. since it should make sure it's not an "optional result". To my knowledge there is no ecosystem-wide wording for it
3. Any alias is an indirection that the reader has to map mentally, while w/o it you can instantly see the type

That is also true. Maybe if we can come up with short, concise and meaningful type name

[comphead]

Thanks @crepererum since it is an api change I'll wait for second reviewer.
@alamb @Jefffrey may help with the second review?

[alamb]

Thank you for this PR @crepererum and @comphead for the review.

I think returning a Result<Option<..>> is somewhat common in the DataFusion APIs so this PR makes the APIs more consistent as well as fixing the issues identified in the PR description

For example in SchemaProvider register_table and deregister_table both return Result<Option<..>>:

    fn register_table(
        &self,
        name: String,
        table: Arc<dyn TableProvider>,
    ) -> Result<Option<Arc<dyn TableProvider>>> {
...

    fn deregister_table(&self, name: &str) -> Result<Option<Arc<dyn TableProvider>>> {
...

I agree that Result<Option<Arc<dyn TableProvider>>> is somewhat 🤮 to read, but as @crepererum and @comphead have pointed out there isn't an obviously better alternative