plusplusjiajia commented on PR #733:
URL: https://github.com/apache/paimon-rust/pull/733#issuecomment-5372408044
> [P2] Keep DataFusion engine registration out of the core Catalog contract
>
> Could we avoid `Catalog::load_table_routing(..., engine_types)`?
`engine_types` is DataFusion/session state, not catalog metadata. Passing it
into `paimon::Catalog` makes loading the same table depend on which
integration/resolvers are currently registered, and it forces third-party
catalogs to implement a DataFusion-specific routing protocol (while the default
implementation ignores the set).
>
> I do not think simply calling `get_table()` and then inspecting
`TableType` in DataFusion is sufficient: by then FS/REST has entered the
concrete Paimon `Table` construction path; REST may acquire a data token and
construct FileIO, and the external-type fail-closed guard must run before that
construction. Calling `get_table_type()` and then `get_table()` would also
fetch metadata twice for native tables.
>
> Could core instead expose an unconditional classification, for example:
>
> ```rust
> async fn load_table(&self, id: &Identifier) -> Result<LoadedTable>;
>
> enum LoadedTable {
> Paimon(Box<Table>),
> External(ExternalTableMetadata),
> }
> ```
>
> The catalog would load metadata once and decide only whether the core
reader can construct the table. Object/Lance/Iceberg would always return
`External`. DataFusion would then look up its own resolver and report a clear
error if none is registered. The existing `get_table()` path can remain
fail-closed for external types. In other words, core owns classification and
stored-metadata validation; DataFusion owns resolver selection, session
semantics, and the read-only wrapper.
>
> This would also remove a hidden invariant in the current public API:
`RoutedTableLoad::engine` does not check `declared.requires_table_engine()`. A
non-DataFusion caller can include a native type in `engine_types` and make the
FS/REST implementations skip native table construction. Correctness currently
relies on the private check in the DataFusion registration path.
>
> This boundary also matches the Java direction more closely: core
dispatches from table metadata without accepting caller-registered engine types.
@JingsongLi Good call on the boundary — engine_types is gone from core.
Catalog::load_table returns LoadedTable::{Paimon, External} and classifies
unconditionally, so the outcome depends only on the table's own metadata. Core
owns classification and the stored-metadata checks; DataFusion owns resolver
selection, session semantics, and the read-only wrapper.
The hidden invariant is closed too: ExternalTableMetadata can only be built
through LoadedTable::external, which rejects any type requires_table_engine
does not accept.
--
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]
