jayzhan211 commented on PR #11516: URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2241115501
> As a follow on PR we may want to consider having `SessionCatalog` also implement `FunctionRegistry` rather than providing access to the hash maps directly > > https://docs.rs/datafusion/latest/datafusion/execution/trait.FunctionRegistry.html > > So that would mean > > ```rust > pub trait CatalogSession: FunctionRegistry + Send + Sync { > ... > } > ``` > > And removing these functions > > However, for this PR I think keeping the API as close as possible to SessionState would be best My concern is still there, we might need more granular trait than single `CatalogSession` for each trait But, the suggestion above inspired me that how we could structure the trait step by step. Given that I'm not confident about `CatalogSession` is the end state of the trait structure, we could have `pub trait TableProviderContext: CatalogSession` for `TableProvider` to ensure we can avoid breaking the `scan` API again if we miss something. If we need a smaller trait, we just change `CatalogSession` to `SomeTrait`. Similar idea applies to `CatalogProvider`, `SchemaProvider` and `FileFormat` if they expects SessionState in the future, avoid using the same general context in every level of trait definition helps potential circular dependency. -- 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]
