gabotechs commented on code in PR #48:
URL: https://github.com/apache/datafusion-iceberg/pull/48#discussion_r4245361898
##########
crates/datafusion/src/table_provider/catalog_table_provider.rs:
##########
@@ -83,11 +88,23 @@ impl IcebergCatalogTableProvider {
.map_err(to_datafusion_error)?,
);
- Ok(IcebergCatalogTableProvider {
+ Ok(Self::new_with_schema(catalog, table_ident, schema))
+ }
+
+ /// Creates a provider for `table_ident` that declares `schema`, without
loading
+ /// the table. For rebuilding a provider in another process from
[`Self::catalog`],
+ /// [`Self::table_ident`] and [`TableProvider::schema`]. `schema` is not
checked
+ /// against the table; scans read its columns by name from the snapshot
they scan.
+ pub fn new_with_schema(
+ catalog: Arc<dyn Catalog>,
+ table_ident: TableIdent,
+ schema: ArrowSchemaRef,
+ ) -> Self {
+ IcebergCatalogTableProvider {
Review Comment:
I don't love the fact that the public API of this project makes incorrect
state representable.
People here can choose to shoot themselves in the foot and pass a `schema`
that has nothing to do with the actual table. This problem goes a bit beyond
just this new constructor method, and it's rooted into the
`IcebergCatalogTableProvider` struct fields themselves.
One thing I notice is that `IcebergCatalogTableProvider` only needs the
`Catalog` itself because it's required for constructing an `IcebergCommitExec`.
It's a bit unfortunate that `Catalog` is dangerously close to a god object in
charge of everything, some interface segregation there could have prevented
from needing to thread it around everywhere.
Given that `Catalog` contains so many methods and not necessarily related
functionality, I wonder if it'd make sense to have it be part of the
DataFusion's `SessionConfig.extensions` and thread it around as part of the
context, rather than manually requiring it to be stored in every struct.
--
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]
