alamb commented on code in PR #16750:
URL: https://github.com/apache/datafusion/pull/16750#discussion_r2202549466
##########
datafusion/catalog/src/default_table_source.rs:
##########
@@ -45,6 +45,26 @@ impl DefaultTableSource {
pub fn new(table_provider: Arc<dyn TableProvider>) -> Self {
Self { table_provider }
}
+
+ /// Attempt to downcast a TableSource to DefaultTableSource and access the
+ /// TableProvider. This will only work with a TableSource created by
DataFusion.
Review Comment:
I don't understand why we need this method when we already have
`source_as_provider`.
If we do need the method, I think it would help to give some more context /
pointers on how to use it and why it is different than the `souce_as_provider`
```suggestion
/// TableProvider. This will only work with a TableSource created by
[`provider_as_source`]
///
/// This is commonly used to downcast the result of
[`provider_as_source`] back to the TableProvider.
```
Bonus points if we could figure out how to do an doc example showing how to
use `unwrap_provider` on the output of `provider_as_source` (though I think
the setup would be pretty tough)
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -117,18 +117,18 @@ pub trait LogicalExtensionCodec: Debug + Send + Sync {
fn try_encode(&self, node: &Extension, buf: &mut Vec<u8>) -> Result<()>;
- fn try_decode_table_provider(
+ fn try_decode_table_source(
Review Comment:
it makes sense to me to have the logical plan serialization be in terms of a
TableSource (the logical part) rather than a Table Provider
https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.TableSource.html
I realize there are no existing docs on this method, but given we are
changing it I think it would be helpful if we added some hints for other readers
Something like
```suggestion
/// Attempts to decode a [`TableSource`]
///
/// Note that the default codec will serialize [`TableProvider`] as a
[`DefaultTableProvider`]
/// you can use [`DefaultTableSource::unwrap_provider`] to recover the
original `TableProvider`.
fn try_decode_table_source(
```
##########
datafusion/catalog/src/default_table_source.rs:
##########
@@ -87,7 +107,7 @@ impl TableSource for DefaultTableSource {
}
}
-/// Wrap TableProvider in TableSource
+/// Wrap a TableProvider as a TableSource.
Review Comment:
```suggestion
/// Wrap a TableProvider as a TableSource.
///
/// See [`source_as_provider`] to retrieve the original
/// TableSource.
```
--
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]
