JingsongLi commented on code in PR #733: URL: https://github.com/apache/paimon-rust/pull/733#discussion_r3827477615
########## crates/paimon/src/spec/table_type.rs: ########## @@ -0,0 +1,134 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::fmt::{Display, Formatter}; +use std::str::FromStr; + +use crate::error::Error; + +/// Type of the table, declared by the `type` table option. +/// +/// Mirrors `org.apache.paimon.TableType`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] +pub enum TableType { + /// Normal Paimon table. + #[default] + Table, + /// A directory containing multiple files of the same format. + FormatTable, + /// A normal Paimon table combined with materialized SQL. + MaterializedTable, + /// A normal Paimon table combined with an object location. + ObjectTable, + /// A lance table, see <https://lancedb.github.io/lance/>. + LanceTable, + /// An iceberg table, see <https://iceberg.apache.org/>. + IcebergTable, +} + +impl TableType { + /// The `type` option value of this table type. + pub fn as_str(&self) -> &'static str { + match self { + TableType::Table => "table", + TableType::FormatTable => "format-table", + TableType::MaterializedTable => "materialized-table", + TableType::ObjectTable => "object-table", + TableType::LanceTable => "lance-table", + TableType::IcebergTable => "iceberg-table", + } + } + + /// Whether the Paimon reader cannot serve this type, so it must be + /// routed to a table engine (see + /// [`Catalog::load_table_routing`](crate::catalog::Catalog::load_table_routing)). + pub fn requires_table_engine(&self) -> bool { + matches!(self, TableType::IcebergTable) Review Comment: [P1] Route every table kind the Rust reader cannot serve This classifies only Iceberg as engine-required, so register_table_engine rejects both ObjectTable and LanceTable as if the ordinary Paimon reader supported them. It does not: the Rust read stack has no Object/Lance provider and its normal scan uses Paimon snapshots (including an empty plan when none exists), while Java CatalogUtils constructs dedicated Object, Lance, and Iceberg table implementations. A real object-table or lance-table can therefore fail or silently appear empty, and callers cannot register the resolver introduced here. Please mark every currently non-native type (at least Object/Lance/Iceberg) as engine-required/fail-closed until native providers exist, and replace the Lance rejection test with routing coverage. ########## crates/integrations/datafusion/src/catalog.rs: ########## @@ -465,9 +603,39 @@ impl SchemaProvider for PaimonSchemaProvider { let schema_force_view_types = self.schema_force_view_types; let identifier = Identifier::new(self.database.clone(), object.table().to_string()); let branch = object.branch().map(str::to_string); + let table_engines: HashMap<PaimonTableType, Arc<dyn TableEngineResolver>> = self + .table_engines + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone(); await_with_runtime(async move { - match catalog.get_table(&identifier).await { - Ok(mut table) => { + let engine_types: HashSet<PaimonTableType> = table_engines.keys().copied().collect(); + match catalog.load_table_routing(&identifier, &engine_types).await { + Ok(paimon::catalog::RoutedTableLoad::Engine(declared)) => { + if branch.is_some() { + return Err(plan_datafusion_err!( + "branches are not supported for '{}' tables ('{}')", + declared, + identifier.full_name() + )); + } + let resolver = table_engines + .get(&declared) + .expect("declared type came from this engine map"); + let resolved = resolver + .resolve_table(identifier.database(), identifier.object()) + .await?; + // Read-only wrap: DML must not reach the engine provider. + Ok(resolved.map(|inner| { Review Comment: [P1] Reject or implement time travel for routed providers Returning this wrapper breaks the existing raw SQLContext::ctx().sql time-travel path: PaimonRelationPlanner cannot downcast it to PaimonTableProvider, returns Original, and DataFusion 54's default relation planner discards TableFactor.version. I reproduced this on this head: after selecting the Databricks dialect, SELECT * FROM ...it VERSION AS OF 999999 succeeded and returned the current two engine rows instead of rejecting the nonexistent version. The high-level SQLContext::sql path fails closed, but the public raw context is intentionally supported and tested for time travel. Please make the relation planner recognize routed providers and explicitly reject VERSION/TIMESTAMP, or extend the resolver contract to carry those selectors. ########## crates/paimon/src/spec/core_options.rs: ########## @@ -624,11 +626,17 @@ impl<'a> CoreOptions<'a> { .unwrap_or(false) } + /// The declared [`TableType`], defaulting to [`TableType::Table`]. + /// Fails on a value this client does not know. + pub fn table_type(&self) -> crate::Result<TableType> { + match self.options.get(TABLE_TYPE_OPTION) { + Some(value) => value.parse(), Review Comment: [P2] Validate the declared type before persisting schema-0 This validation currently first runs while loading/routing. FileSystemCatalog::create_table still creates the directory and writes schema-0 without calling table_type(), and the bundled REST server delegates to it. Thus CREATE TABLE ... WITH ('type'='iceberg-tabel') returns success but every later get_table/routing call fails with unknown table type, leaving an unusable catalog entry. Please invoke this parsing from shared create/schema validation before any metadata write and add filesystem/REST tests that assert rejection leaves no table artifacts. ########## crates/paimon/src/catalog/filesystem.rs: ########## @@ -297,30 +354,26 @@ impl Catalog for FileSystemCatalog { } async fn get_table(&self, identifier: &Identifier) -> Result<Table> { - identifier.validate()?; - - let table_path = self.table_path(identifier); + let (table_path, schema) = self.fetch_table_schema(identifier).await?; + self.build_table(identifier, table_path, schema) + } - if !self.table_exists(identifier).await? { - return Err(Error::TableNotExist { - full_name: identifier.full_name(), - }); + async fn load_table_routing( + &self, + identifier: &Identifier, + engine_types: &std::collections::HashSet<TableType>, + ) -> Result<crate::catalog::RoutedTableLoad> { + let (table_path, schema) = self.fetch_table_schema(identifier).await?; + let options = CoreOptions::new(schema.options()); + let declared = options.table_type()?; Review Comment: [P1] Keep type immutable before trusting it for routing This makes the latest schema's type an authoritative engine switch, but TableSchema::apply_changes still accepts arbitrary SetOption/RemoveOption, and SQL exposes that through ALTER TABLE ... SET TBLPROPERTIES. A populated Paimon table can be changed to type=iceberg-table; with a resolver, later reads are redirected to a same-named engine table, and without one the existing snapshots become inaccessible. Java SchemaManager explicitly rejects semantic changes/removal of CoreOptions.TYPE, even before snapshots exist. Please enforce the same invariant before saving a schema (allowing only case-insensitive/default-equivalent no-ops) and cover filesystem plus REST ALTER paths. -- 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]
