From 3af88be37c024ece973ec21c0cb5b1eefb016c1d Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Wed, 19 Aug 2026 06:40:54 -0400 Subject: [PATCH 1/6] feat: route declared non-Paimon table types to pluggable engines --- crates/integrations/datafusion/src/catalog.rs | 255 +++++++- crates/integrations/datafusion/src/lib.rs | 2 +- .../datafusion/src/sql_context.rs | 26 + .../datafusion/tests/table_type_routing.rs | 554 ++++++++++++++++++ crates/paimon/src/catalog/mod.rs | 30 + .../paimon/src/catalog/rest/rest_catalog.rs | 29 + crates/paimon/src/spec/core_options.rs | 20 + crates/paimon/src/table/rest_env.rs | 46 +- crates/paimon/tests/rest_catalog_test.rs | 115 ++++ 9 files changed, 1050 insertions(+), 27 deletions(-) create mode 100644 crates/integrations/datafusion/tests/table_type_routing.rs diff --git a/crates/integrations/datafusion/src/catalog.rs b/crates/integrations/datafusion/src/catalog.rs index 83ef810fa..33e4ae662 100644 --- a/crates/integrations/datafusion/src/catalog.rs +++ b/crates/integrations/datafusion/src/catalog.rs @@ -43,6 +43,95 @@ use crate::{BlobReaderRegistry, DynamicOptions}; pub(crate) type SessionStateProvider = Arc Option + Send + Sync>; +/// Engine registry shared between the catalog provider and its schema +/// providers, so registrations stay visible to schemas obtained earlier. +type TableEngines = Arc>>>; + +/// Resolves tables owned by a non-Paimon engine, selected by the declared +/// `type` table option (see [`PaimonCatalogProvider::register_table_engine`]). +/// `Ok(None)` means not found; errors are propagated verbatim so an engine +/// failure never masquerades as a missing table. +#[async_trait] +pub trait TableEngineResolver: Debug + Send + Sync { + /// Resolve `database.table` to the engine's table provider. + async fn resolve_table( + &self, + database: &str, + table: &str, + ) -> DFResult>>; +} + +/// Read-only wrapper around an engine-resolved provider: reads delegate, +/// DML is rejected. Keeps routed tables on the same fail-closed stance as +/// the raw `get_table` paths even when the engine's provider is writable. +#[derive(Debug)] +struct ReadOnlyTableProvider { + inner: Arc, + declared: String, + table_name: String, +} + +#[async_trait] +impl TableProvider for ReadOnlyTableProvider { + fn schema(&self) -> datafusion::arrow::datatypes::SchemaRef { + self.inner.schema() + } + + fn constraints(&self) -> Option<&datafusion::common::Constraints> { + self.inner.constraints() + } + + fn table_type(&self) -> TableType { + self.inner.table_type() + } + + fn get_table_definition(&self) -> Option<&str> { + self.inner.get_table_definition() + } + + fn get_logical_plan(&self) -> Option> { + self.inner.get_logical_plan() + } + + fn get_column_default(&self, column: &str) -> Option<&Expr> { + self.inner.get_column_default(column) + } + + async fn scan( + &self, + state: &dyn datafusion::catalog::Session, + projection: Option<&Vec>, + filters: &[Expr], + limit: Option, + ) -> DFResult> { + self.inner.scan(state, projection, filters, limit).await + } + + fn supports_filters_pushdown( + &self, + filters: &[&Expr], + ) -> DFResult> { + self.inner.supports_filters_pushdown(filters) + } + + fn statistics(&self) -> Option { + self.inner.statistics() + } + + async fn insert_into( + &self, + _state: &dyn datafusion::catalog::Session, + _input: Arc, + _insert_op: datafusion::logical_expr::dml::InsertOp, + ) -> DFResult> { + Err(plan_datafusion_err!( + "write is not supported for routed '{}' tables ('{}')", + self.declared, + self.table_name + )) + } +} + /// Provides an interface to manage and access multiple schemas (databases) /// within a Paimon [`Catalog`]. /// @@ -65,6 +154,9 @@ pub struct PaimonCatalogProvider { blob_reader_registry: BlobReaderRegistry, session_state: Option, schema_force_view_types: bool, + /// Engines for non-Paimon table types, keyed by the declared `type` table + /// option. Same poison-recovery stance as `temp_tables`. + table_engines: TableEngines, } impl Debug for PaimonCatalogProvider { @@ -90,6 +182,7 @@ impl PaimonCatalogProvider { blob_reader_registry, session_state, schema_force_view_types: true, + table_engines: Arc::new(RwLock::new(HashMap::new())), } } @@ -102,24 +195,43 @@ impl PaimonCatalogProvider { self.schema_force_view_types = schema_force_view_types; self } -} -impl CatalogProvider for PaimonCatalogProvider { - fn schema_names(&self) -> Vec { - let catalog = Arc::clone(&self.catalog); - block_on_with_runtime( - async move { - catalog.list_databases().await.unwrap_or_else(|e| { - log::error!("failed to list databases: {e}"); - vec![] - }) - }, - "paimon catalog access thread panicked", - ) + /// Register an engine for a non-Paimon table type (e.g. `iceberg-table`). + /// References whose metadata declares that type resolve through + /// `resolver`; everything else takes the Paimon path unchanged. Kept + /// inside the provider so the registered catalog type never changes and + /// downcast-based paths (temp tables, time travel) keep working. + pub fn register_table_engine( + &self, + table_type: impl Into, + resolver: Arc, + ) -> DFResult<()> { + // Metadata publishes lowercase TableType values. + let table_type = table_type.into().to_ascii_lowercase(); + // A Paimon-managed type would be split between engines: reads via + // the resolver, raw get_table paths via Paimon. + if !paimon::spec::is_non_paimon_table_type(&table_type) { + return Err(plan_datafusion_err!( + "table type '{table_type}' is Paimon-managed and cannot be routed to a \ + table engine; routable types: {:?}", + paimon::spec::NON_PAIMON_TABLE_TYPES + )); + } + self.table_engines + .write() + .unwrap_or_else(|e| e.into_inner()) + .insert(table_type, resolver); + Ok(()) } - fn schema(&self, name: &str) -> Option> { + fn table_engines(&self) -> TableEngines { + Arc::clone(&self.table_engines) + } + + /// The Paimon-side schema resolution. + fn paimon_schema(&self, name: &str) -> Option> { let catalog = Arc::clone(&self.catalog); + let table_engines = self.table_engines(); let dynamic_options = Arc::clone(&self.dynamic_options); let blob_reader_registry = self.blob_reader_registry.clone(); let catalog_name = self.catalog_name.clone(); @@ -145,7 +257,8 @@ impl CatalogProvider for PaimonCatalogProvider { blob_reader_registry, session_state, ) - .with_schema_force_view_types(schema_force_view_types), + .with_schema_force_view_types(schema_force_view_types) + .with_table_engines(Arc::clone(&table_engines)), ) as Arc), Err(paimon::Error::DatabaseNotExist { .. }) => { if temp_provider.is_some() { @@ -159,7 +272,8 @@ impl CatalogProvider for PaimonCatalogProvider { blob_reader_registry, session_state, ) - .with_schema_force_view_types(schema_force_view_types), + .with_schema_force_view_types(schema_force_view_types) + .with_table_engines(Arc::clone(&table_engines)), ) as Arc) } else { None @@ -174,6 +288,25 @@ impl CatalogProvider for PaimonCatalogProvider { "paimon catalog access thread panicked", ) } +} + +impl CatalogProvider for PaimonCatalogProvider { + fn schema_names(&self) -> Vec { + let catalog = Arc::clone(&self.catalog); + block_on_with_runtime( + async move { + catalog.list_databases().await.unwrap_or_else(|e| { + log::error!("failed to list databases: {e}"); + vec![] + }) + }, + "paimon catalog access thread panicked", + ) + } + + fn schema(&self, name: &str) -> Option> { + self.paimon_schema(name) + } fn register_schema( &self, @@ -343,6 +476,8 @@ pub struct PaimonSchemaProvider { blob_reader_registry: BlobReaderRegistry, session_state: Option, schema_force_view_types: bool, + /// Engines for non-Paimon table types; empty without routing. + table_engines: TableEngines, } impl Debug for PaimonSchemaProvider { @@ -375,6 +510,7 @@ impl PaimonSchemaProvider { blob_reader_registry, session_state, schema_force_view_types: true, + table_engines: Arc::new(RwLock::new(HashMap::new())), } } @@ -382,6 +518,11 @@ impl PaimonSchemaProvider { self.schema_force_view_types = schema_force_view_types; self } + + pub(crate) fn with_table_engines(mut self, table_engines: TableEngines) -> Self { + self.table_engines = table_engines; + self + } } #[async_trait] @@ -465,9 +606,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> = 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 non_paimon: HashSet = table_engines.keys().cloned().collect(); + match catalog.load_table_routing(&identifier, &non_paimon).await { + Ok(paimon::catalog::RoutedTableLoad::NonPaimon(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| { + Arc::new(ReadOnlyTableProvider { + inner, + declared, + table_name: identifier.full_name(), + }) as Arc + })) + } + Ok(paimon::catalog::RoutedTableLoad::Paimon(table)) => { + let mut table = *table; if let Some(branch) = branch.as_deref() { table = table .copy_with_branch(branch) @@ -524,7 +695,8 @@ impl SchemaProvider for PaimonSchemaProvider { identifier.full_name() ) })?; - validate_view_dependencies(&catalog, &catalog_name, &view).await?; + validate_view_dependencies(&catalog, &catalog_name, &view, &non_paimon) + .await?; let mut state = session_state .and_then(|provider| provider()) .ok_or_else(|| { @@ -606,15 +778,47 @@ impl SchemaProvider for PaimonSchemaProvider { let is_branches_table = object .system_table() .is_some_and(|name| name.eq_ignore_ascii_case("branches")); + let has_system_suffix = object.system_table().is_some(); + let engines: HashMap> = self + .table_engines + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone(); block_on_with_runtime( async move { - match catalog.get_table(&identifier).await { - Ok(table) => { + let non_paimon: HashSet = engines.keys().cloned().collect(); + match catalog.load_table_routing(&identifier, &non_paimon).await { + Ok(paimon::catalog::RoutedTableLoad::NonPaimon(declared)) => { + // Branches and system tables are Paimon-only; + // `table()` rejects them for routed tables. + if branch.is_some() || has_system_suffix { + return false; + } + match engines.get(&declared) { + Some(resolver) => match resolver + .resolve_table(identifier.database(), identifier.object()) + .await + { + Ok(table) => table.is_some(), + // Report failures as existing so `table()` + // surfaces the real error. + Err(err) => { + log::warn!( + "failed to probe engine table existence for '{}': {err}", + identifier.full_name() + ); + true + } + }, + None => false, + } + } + Ok(paimon::catalog::RoutedTableLoad::Paimon(table)) => { if let Some(branch) = branch.as_deref() { if is_branches_table { return true; } - table.copy_with_branch(branch).await.is_ok() + (*table).copy_with_branch(branch).await.is_ok() } else { true } @@ -712,6 +916,7 @@ async fn validate_view_dependencies( catalog: &Arc, catalog_name: &str, root: &View, + non_paimon_types: &HashSet, ) -> DFResult<()> { let mut queue = VecDeque::from([root.clone()]); let mut loaded = HashSet::from([root.identifier().clone()]); @@ -721,7 +926,11 @@ async fn validate_view_dependencies( let candidates = view_relation_identifiers(&view, catalog_name)?; let mut view_dependencies = Vec::new(); for identifier in candidates { - match catalog.get_table(&identifier).await { + // Routed non-Paimon tables count as existing dependencies. + match catalog + .load_table_routing(&identifier, non_paimon_types) + .await + { Ok(_) => continue, Err(paimon::Error::TableNotExist { .. }) | Err(paimon::Error::Unsupported { .. }) => {} diff --git a/crates/integrations/datafusion/src/lib.rs b/crates/integrations/datafusion/src/lib.rs index d9044fbc9..784215e0b 100644 --- a/crates/integrations/datafusion/src/lib.rs +++ b/crates/integrations/datafusion/src/lib.rs @@ -74,7 +74,7 @@ pub(crate) type DynamicOptions = Arc>>; pub use blob_reader::BlobReaderRegistry; pub use blob_view::register_blob_view; -pub use catalog::{PaimonCatalogProvider, PaimonSchemaProvider}; +pub use catalog::{PaimonCatalogProvider, PaimonSchemaProvider, TableEngineResolver}; pub use error::to_datafusion_error; #[cfg(feature = "fulltext")] pub use full_text_search::{register_full_text_search, FullTextSearchFunction}; diff --git a/crates/integrations/datafusion/src/sql_context.rs b/crates/integrations/datafusion/src/sql_context.rs index ae02f0a95..06d215250 100644 --- a/crates/integrations/datafusion/src/sql_context.rs +++ b/crates/integrations/datafusion/src/sql_context.rs @@ -348,6 +348,32 @@ impl SQLContext { paimon_provider.register_temp_table(&database, &table_name, table) } + /// Whether `name` was registered through the Paimon registration path. + pub fn is_paimon_catalog(&self, name: &str) -> bool { + self.catalogs.contains_key(name) + } + + /// Register an engine for a non-Paimon table type (the declared `type` + /// table option, e.g. `iceberg-table`) on a registered Paimon catalog. + /// See [`crate::catalog::PaimonCatalogProvider::register_table_engine`]. + pub fn register_catalog_table_engine( + &self, + catalog_name: &str, + table_type: &str, + resolver: Arc, + ) -> DFResult<()> { + let catalog_provider = self + .ctx + .catalog(catalog_name) + .ok_or_else(|| DataFusionError::Plan(format!("Unknown catalog '{catalog_name}'")))?; + let paimon_provider = catalog_provider + .downcast_ref::() + .ok_or_else(|| { + DataFusionError::Plan(format!("Catalog '{catalog_name}' is not a Paimon catalog")) + })?; + paimon_provider.register_table_engine(table_type, resolver) + } + /// Deregisters a temporary table or view. /// /// Accepts the same flexible name format as `register_temp_table`. diff --git a/crates/integrations/datafusion/tests/table_type_routing.rs b/crates/integrations/datafusion/tests/table_type_routing.rs new file mode 100644 index 000000000..f9d2a77c1 --- /dev/null +++ b/crates/integrations/datafusion/tests/table_type_routing.rs @@ -0,0 +1,554 @@ +// 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. + +//! Declared-type table routing: tables declared as a non-Paimon type (e.g. +//! `iceberg-table`) resolve through the registered engine; everything else +//! takes the Paimon path unchanged. + +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use async_trait::async_trait; +use datafusion::arrow::array::{Array, Int32Array, StringArray}; +use datafusion::arrow::datatypes::{DataType, Field, Schema as ArrowSchema}; +use datafusion::arrow::record_batch::RecordBatch; +use datafusion::datasource::{MemTable, TableProvider}; +use datafusion::error::{DataFusionError, Result as DFResult}; +use paimon::catalog::{Catalog, Database, Identifier, RoutedTableLoad}; +use paimon::spec::{Schema as PaimonSchema, SchemaChange}; +use paimon::table::Table; +use paimon::{CatalogOptions, FileSystemCatalog, Options, Result as PaimonResult}; +use paimon_datafusion::{SQLContext, TableEngineResolver}; +use tempfile::TempDir; + +const CATALOG: &str = "cat"; +const DB: &str = "shared_db"; +const ICEBERG_TABLE_TYPE: &str = "iceberg-table"; + +/// A filesystem-backed catalog that declares table types the way a REST +/// catalog does through the `type` table option. +#[derive(Debug)] +struct TypedTestCatalog { + inner: Arc, + declared_types: HashMap, +} + +#[async_trait] +impl Catalog for TypedTestCatalog { + async fn list_databases(&self) -> PaimonResult> { + self.inner.list_databases().await + } + + async fn create_database( + &self, + name: &str, + ignore_if_exists: bool, + properties: HashMap, + ) -> PaimonResult<()> { + self.inner + .create_database(name, ignore_if_exists, properties) + .await + } + + async fn get_database(&self, name: &str) -> PaimonResult { + self.inner.get_database(name).await + } + + async fn drop_database( + &self, + name: &str, + ignore_if_not_exists: bool, + cascade: bool, + ) -> PaimonResult<()> { + self.inner + .drop_database(name, ignore_if_not_exists, cascade) + .await + } + + async fn get_table(&self, identifier: &Identifier) -> PaimonResult { + // Mirror the REST catalog: declared-non-Paimon tables fail closed. + if let Some(declared) = self.declared_types.get(identifier.object()) { + return Err(paimon::Error::Unsupported { + message: format!( + "table '{}' is declared '{declared}' and cannot be read as a Paimon table", + identifier.full_name() + ), + }); + } + self.inner.get_table(identifier).await + } + + async fn load_table_routing( + &self, + identifier: &Identifier, + non_paimon_types: &HashSet, + ) -> PaimonResult { + if let Some(declared) = self.declared_types.get(identifier.object()) { + let declared = declared.to_ascii_lowercase(); + if non_paimon_types.contains(&declared) { + return Ok(RoutedTableLoad::NonPaimon(declared)); + } + } + Ok(RoutedTableLoad::Paimon(Box::new( + self.get_table(identifier).await?, + ))) + } + + async fn list_tables(&self, database_name: &str) -> PaimonResult> { + let mut names = self.inner.list_tables(database_name).await?; + names.extend(self.declared_types.keys().cloned()); + Ok(names) + } + + async fn create_table( + &self, + identifier: &Identifier, + creation: PaimonSchema, + ignore_if_exists: bool, + ) -> PaimonResult<()> { + self.inner + .create_table(identifier, creation, ignore_if_exists) + .await + } + + async fn drop_table( + &self, + identifier: &Identifier, + ignore_if_not_exists: bool, + ) -> PaimonResult<()> { + self.inner + .drop_table(identifier, ignore_if_not_exists) + .await + } + + async fn rename_table( + &self, + from: &Identifier, + to: &Identifier, + ignore_if_not_exists: bool, + ) -> PaimonResult<()> { + self.inner + .rename_table(from, to, ignore_if_not_exists) + .await + } + + async fn alter_table( + &self, + identifier: &Identifier, + changes: Vec, + ignore_if_not_exists: bool, + ) -> PaimonResult<()> { + self.inner + .alter_table(identifier, changes, ignore_if_not_exists) + .await + } +} + +/// A stand-in engine: serves a fixed two-row in-memory table for `it`. +#[derive(Debug)] +struct FakeEngineResolver; + +#[async_trait] +impl TableEngineResolver for FakeEngineResolver { + async fn resolve_table( + &self, + _database: &str, + table: &str, + ) -> DFResult>> { + if table != "it" { + return Ok(None); + } + let schema = Arc::new(ArrowSchema::new(vec![ + Field::new("id", DataType::Int32, false), + Field::new("payload", DataType::Utf8, false), + ])); + let batch = RecordBatch::try_new( + Arc::clone(&schema), + vec![ + Arc::new(Int32Array::from(vec![1, 3])), + Arc::new(StringArray::from(vec!["x", "y"])), + ], + ) + .map_err(DataFusionError::from)?; + let table = MemTable::try_new(schema, vec![vec![batch]])?; + Ok(Some(Arc::new(table))) + } +} + +struct TestEnv { + // Owns the warehouse directory for the duration of the test. + _paimon_dir: TempDir, + ctx: SQLContext, +} + +/// One SQLContext with catalog `cat`: a Paimon table `cat.shared_db.pt` +/// (two rows, two snapshots) plus a declared `iceberg-table` +/// `cat.shared_db.it` routed to a fake engine. +async fn setup() -> TestEnv { + let paimon_dir = TempDir::new().unwrap(); + let warehouse = format!("file://{}", paimon_dir.path().display()); + let mut options = Options::new(); + options.set(CatalogOptions::WAREHOUSE, warehouse); + let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); + let typed_catalog = Arc::new(TypedTestCatalog { + inner: fs_catalog, + declared_types: HashMap::from([ + ("it".to_string(), ICEBERG_TABLE_TYPE.to_string()), + // Unknown to the engine: existence mirrors the resolver's miss. + ("ghost".to_string(), ICEBERG_TABLE_TYPE.to_string()), + // Write-statement target: raw get_table paths fail closed. + ("ft".to_string(), ICEBERG_TABLE_TYPE.to_string()), + ]), + }); + let mut ctx = SQLContext::new(); + ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); + ctx.sql(&format!("CREATE SCHEMA {CATALOG}.{DB}")) + .await + .unwrap(); + ctx.sql(&format!( + "CREATE TABLE {CATALOG}.{DB}.pt (id INT NOT NULL, name STRING)" + )) + .await + .unwrap(); + // Two separate INSERTs -> two snapshots, so time travel has history. + for stmt in [ + format!("INSERT INTO {CATALOG}.{DB}.pt VALUES (1, 'a')"), + format!("INSERT INTO {CATALOG}.{DB}.pt VALUES (2, 'b')"), + ] { + ctx.sql(&stmt).await.unwrap().collect().await.unwrap(); + } + + ctx.register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(FakeEngineResolver)) + .unwrap(); + + TestEnv { + _paimon_dir: paimon_dir, + ctx, + } +} + +fn column_i32(batches: &[RecordBatch]) -> Vec { + batches + .iter() + .flat_map(|b| { + let col = b.column(0).as_any().downcast_ref::().unwrap(); + (0..col.len()).map(|i| col.value(i)).collect::>() + }) + .collect() +} + +#[tokio::test] +async fn paimon_path_still_serves_paimon_tables() { + let env = setup().await; + let batches = env + .ctx + .sql(&format!("SELECT id FROM {CATALOG}.{DB}.pt ORDER BY id")) + .await + .unwrap() + .collect() + .await + .unwrap(); + assert_eq!(column_i32(&batches), vec![1, 2]); +} + +#[tokio::test] +async fn declared_non_paimon_table_routes_to_engine() { + let env = setup().await; + let df = env + .ctx + .sql(&format!("SELECT id, payload FROM {CATALOG}.{DB}.it")) + .await + .unwrap(); + // Schema comes from the engine — `payload` only exists there. + let names: Vec = df + .schema() + .fields() + .iter() + .map(|f| f.name().clone()) + .collect(); + assert_eq!(names, vec!["id".to_string(), "payload".to_string()]); + let batches = df.collect().await.unwrap(); + let rows: usize = batches.iter().map(|b| b.num_rows()).sum(); + assert_eq!(rows, 2); +} + +#[tokio::test] +async fn cross_engine_join_plans_and_runs() { + let env = setup().await; + let batches = env + .ctx + .sql(&format!( + "SELECT p.id FROM {CATALOG}.{DB}.pt p JOIN {CATALOG}.{DB}.it i ON p.id = i.id" + )) + .await + .unwrap() + .collect() + .await + .unwrap(); + // pt has ids {1,2}; the engine table has {1,3}. + assert_eq!(column_i32(&batches), vec![1]); +} + +#[tokio::test] +async fn missing_table_still_errors() { + let env = setup().await; + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.does_not_exist")) + .await + else { + panic!("query against a missing table must fail"); + }; + let msg = err.to_string().to_lowercase(); + assert!( + msg.contains("does_not_exist") || msg.contains("not found"), + "{msg}" + ); +} + +// Downcast-based paths — time travel, temp tables — must keep working +// with engines registered. +#[tokio::test] +async fn time_travel_still_works_with_engines_registered() { + let env = setup().await; + let batches = env + .ctx + .sql(&format!("SELECT id FROM {CATALOG}.{DB}.pt VERSION AS OF 1")) + .await + .unwrap() + .collect() + .await + .unwrap(); + // Snapshot 1 holds only the first INSERT. + assert_eq!(column_i32(&batches), vec![1]); +} + +#[tokio::test] +async fn temp_tables_still_work_with_engines_registered() { + let env = setup().await; + let schema = Arc::new(datafusion::arrow::datatypes::Schema::new(vec![Field::new( + "id", + DataType::Int32, + false, + )])); + let mem = MemTable::try_new(schema, vec![vec![]]).unwrap(); + env.ctx + .register_temp_table(format!("{CATALOG}.{DB}.tmp_t"), Arc::new(mem)) + .expect("temp table registration must survive engine registration"); + assert!(env + .ctx + .temp_table_exist(format!("{CATALOG}.{DB}.tmp_t")) + .unwrap()); +} + +#[tokio::test] +async fn schemas_and_ddl_behave_with_engines_registered() { + let env = setup().await; + let provider = env.ctx.ctx().catalog(CATALOG).unwrap(); + assert!(provider.schema(DB).is_some()); + assert!(provider.schema("no_such_db").is_none()); + env.ctx + .sql(&format!("CREATE SCHEMA {CATALOG}.fresh_db")) + .await + .expect("CREATE SCHEMA must work with engines registered"); + env.ctx + .sql(&format!( + "CREATE TABLE {CATALOG}.fresh_db.t (id INT NOT NULL)" + )) + .await + .expect("CREATE TABLE in the fresh schema must work"); +} + +#[tokio::test] +async fn table_names_come_from_the_catalog_listing() { + let env = setup().await; + let provider = env.ctx.ctx().catalog(CATALOG).unwrap(); + let schema = provider.schema(DB).unwrap(); + let mut names = schema.table_names(); + names.sort(); + names.dedup(); + assert!(names.contains(&"pt".to_string()), "{names:?}"); + assert!(names.contains(&"it".to_string()), "{names:?}"); +} + +// Engine failures surface as the engine's error, never as "not found". +#[derive(Debug)] +struct BrokenResolver; + +#[async_trait] +impl TableEngineResolver for BrokenResolver { + async fn resolve_table( + &self, + _database: &str, + _table: &str, + ) -> DFResult>> { + Err(DataFusionError::Execution( + "engine backend exploded".to_string(), + )) + } +} + +#[tokio::test] +async fn engine_errors_are_surfaced() { + let env = setup().await; + env.ctx + .register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(BrokenResolver)) + .unwrap(); + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + else { + panic!("broken engine must fail loudly"); + }; + let msg = err.to_string(); + assert!(msg.contains("engine backend exploded"), "{msg}"); +} + +// A declared non-Paimon type with no registered engine takes the Paimon path. +#[tokio::test] +async fn undeclared_engine_type_takes_paimon_path() { + let paimon_dir = TempDir::new().unwrap(); + let warehouse = format!("file://{}", paimon_dir.path().display()); + let mut options = Options::new(); + options.set(CatalogOptions::WAREHOUSE, warehouse); + let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); + let typed_catalog = Arc::new(TypedTestCatalog { + inner: fs_catalog, + declared_types: HashMap::from([("ot".to_string(), "object-table".to_string())]), + }); + let mut ctx = SQLContext::new(); + ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); + ctx.sql(&format!("CREATE SCHEMA {CATALOG}.{DB}")) + .await + .unwrap(); + // No engine registered at all: `ot` resolves through Paimon and fails as + // a plain missing table, exactly like before routing existed. + let Err(err) = ctx.sql(&format!("SELECT * FROM {CATALOG}.{DB}.ot")).await else { + panic!("undeclared engine type must fall through to Paimon"); + }; + let msg = err.to_string().to_lowercase(); + assert!(msg.contains("ot") || msg.contains("not found"), "{msg}"); +} + +// Write statements against routed tables must fail closed. +#[tokio::test] +async fn writes_to_routed_tables_fail_closed() { + let env = setup().await; + let Err(err) = env + .ctx + .sql(&format!("UPDATE {CATALOG}.{DB}.ft SET id = 1")) + .await + else { + panic!("UPDATE on a routed table must fail"); + }; + let msg = err.to_string(); + assert!(msg.contains("cannot be read as a Paimon table"), "{msg}"); + assert!(msg.contains(ICEBERG_TABLE_TYPE), "{msg}"); +} + +// System tables are Paimon-only; routed tables get a clear error. +#[tokio::test] +async fn system_tables_on_routed_tables_error() { + let env = setup().await; + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.\"it$snapshots\"")) + .await + else { + panic!("system table on a routed table must fail"); + }; + let msg = err.to_string(); + assert!(msg.contains("cannot be read as a Paimon table"), "{msg}"); +} + +// Existence mirrors resolution: engine hit -> true, miss -> false. +#[tokio::test] +async fn table_exist_mirrors_the_resolver() { + let env = setup().await; + let provider = env.ctx.ctx().catalog(CATALOG).unwrap(); + let schema = provider.schema(DB).unwrap(); + assert!(schema.table_exist("it")); + assert!(!schema.table_exist("ghost")); + assert!(!schema.table_exist("it$snapshots")); +} + +// Only declared non-Paimon table types may be routed. +#[tokio::test] +async fn paimon_managed_types_cannot_be_routed() { + let env = setup().await; + let Err(err) = + env.ctx + .register_catalog_table_engine(CATALOG, "lance-table", Arc::new(FakeEngineResolver)) + else { + panic!("registering an engine for a Paimon-managed type must fail"); + }; + let msg = err.to_string(); + assert!(msg.contains("Paimon-managed"), "{msg}"); +} + +// Registration canonicalizes the type key. +#[tokio::test] +async fn mixed_case_registration_still_routes() { + let env = setup().await; + env.ctx + .register_catalog_table_engine(CATALOG, "ICEBERG-TABLE", Arc::new(FakeEngineResolver)) + .unwrap(); + let batches = env + .ctx + .sql(&format!("SELECT id FROM {CATALOG}.{DB}.it ORDER BY id")) + .await + .unwrap() + .collect() + .await + .unwrap(); + assert_eq!(column_i32(&batches), vec![1, 3]); +} + +// Resolver failures must not masquerade as misses in existence checks. +#[tokio::test] +async fn table_exist_preserves_resolver_failures() { + let env = setup().await; + env.ctx + .register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(BrokenResolver)) + .unwrap(); + let provider = env.ctx.ctx().catalog(CATALOG).unwrap(); + let schema = provider.schema(DB).unwrap(); + // Conservatively "exists"; the follow-up table() surfaces the error. + assert!(schema.table_exist("it")); +} + +// DML against a routed table is rejected even when the engine's provider +// is writable (the fake resolver returns a writable MemTable). +#[tokio::test] +async fn insert_into_routed_table_is_rejected() { + let env = setup().await; + let err = async { + env.ctx + .sql(&format!("INSERT INTO {CATALOG}.{DB}.it VALUES (9, 'z')")) + .await? + .collect() + .await + } + .await + .expect_err("insert into a routed table must fail"); + assert!( + err.to_string() + .contains("write is not supported for routed 'iceberg-table' tables"), + "{err}" + ); +} diff --git a/crates/paimon/src/catalog/mod.rs b/crates/paimon/src/catalog/mod.rs index 38a414b97..0aadaef67 100644 --- a/crates/paimon/src/catalog/mod.rs +++ b/crates/paimon/src/catalog/mod.rs @@ -265,6 +265,17 @@ use crate::api::PagedList; use crate::spec::{Partition, Schema, SchemaChange}; use crate::table::Table; +/// Outcome of [`Catalog::load_table_routing`]. +#[derive(Debug)] +pub enum RoutedTableLoad { + /// The table was constructed as a Paimon table (boxed: `Table` is much + /// larger than the other variant). + Paimon(Box
), + /// The declared table type matched `non_paimon_types`; construction was + /// skipped. + NonPaimon(String), +} + /// Catalog API for reading and writing metadata (databases, tables) in Paimon. /// /// Corresponds to [org.apache.paimon.catalog.Catalog](https://github.com/apache/paimon/blob/release-1.3/paimon-core/src/main/java/org/apache/paimon/catalog/Catalog.java). @@ -323,6 +334,25 @@ pub trait Catalog: Send + Sync { /// * [`crate::Error::TableNotExist`] - table does not exist. async fn get_table(&self, identifier: &Identifier) -> Result
; + /// Load a table, or return only its declared type (the `type` table + /// option) when it matches one of `non_paimon_types` — skipping + /// construction and any token/FileIO I/O. One metadata round-trip + /// either way. The default implementation always constructs, for + /// catalogs without a table-type concept. + /// + /// # Errors + /// Same as [`Catalog::get_table`]. + async fn load_table_routing( + &self, + identifier: &Identifier, + non_paimon_types: &std::collections::HashSet, + ) -> Result { + let _ = non_paimon_types; + Ok(RoutedTableLoad::Paimon(Box::new( + self.get_table(identifier).await?, + ))) + } + /// List table names in a database. System tables are not listed. /// /// # Errors diff --git a/crates/paimon/src/catalog/rest/rest_catalog.rs b/crates/paimon/src/catalog/rest/rest_catalog.rs index 401545cb8..662a8650c 100644 --- a/crates/paimon/src/catalog/rest/rest_catalog.rs +++ b/crates/paimon/src/catalog/rest/rest_catalog.rs @@ -215,6 +215,35 @@ impl Catalog for RESTCatalog { .await } + async fn load_table_routing( + &self, + identifier: &Identifier, + non_paimon_types: &std::collections::HashSet, + ) -> Result { + let response = RESTEnv::fetch_table_response(identifier, &self.api).await?; + if let Some(schema) = response.schema.as_ref() { + if let Some(declared) = schema.options().get("type") { + let declared = declared.to_ascii_lowercase(); + if non_paimon_types.contains(&declared) { + // Neither this client nor an engine resolver can enforce + // the server-side row filter / column masking. + crate::spec::CoreOptions::new(schema.options()).ensure_read_authorized()?; + return Ok(crate::catalog::RoutedTableLoad::NonPaimon(declared)); + } + } + } + RESTEnv::build_table( + identifier, + response, + self.api.clone(), + self.options.clone(), + self.data_token_enabled, + self.local_cache.clone(), + ) + .await + .map(|table| crate::catalog::RoutedTableLoad::Paimon(Box::new(table))) + } + async fn list_tables(&self, database_name: &str) -> Result> { self.api .list_tables(database_name) diff --git a/crates/paimon/src/spec/core_options.rs b/crates/paimon/src/spec/core_options.rs index e9f34332d..606af71c0 100644 --- a/crates/paimon/src/spec/core_options.rs +++ b/crates/paimon/src/spec/core_options.rs @@ -73,6 +73,18 @@ const STATS_MODE_SUFFIX: &str = "stats-mode"; const ROW_TRACKING_ENABLED_OPTION: &str = "row-tracking.enabled"; const CLUSTERING_INCREMENTAL_OPTION: &str = "clustering.incremental"; pub(crate) const TABLE_TYPE_OPTION: &str = "type"; + +/// Declared table types a Paimon reader cannot serve; the only types +/// accepted for table-engine routing. The Java side loads these as +/// metadata-only tables (e.g. `IcebergTable`). +pub const NON_PAIMON_TABLE_TYPES: &[&str] = &["iceberg-table"]; + +/// Whether `table_type` is one of [`NON_PAIMON_TABLE_TYPES`]. +pub fn is_non_paimon_table_type(table_type: &str) -> bool { + NON_PAIMON_TABLE_TYPES + .iter() + .any(|declared| table_type.eq_ignore_ascii_case(declared)) +} pub(crate) const FORMAT_TABLE_TYPE: &str = "format-table"; pub(crate) const PATH_OPTION: &str = "path"; const MANIFEST_COMPRESSION_OPTION: &str = "manifest.compression"; @@ -631,6 +643,14 @@ impl<'a> CoreOptions<'a> { .unwrap_or(false) } + /// Whether the declared table type is one of [`NON_PAIMON_TABLE_TYPES`]. + pub fn is_non_paimon_table(&self) -> bool { + self.options + .get(TABLE_TYPE_OPTION) + .map(|value| is_non_paimon_table_type(value)) + .unwrap_or(false) + } + pub fn path(&self) -> Option<&str> { self.options.get(PATH_OPTION).map(String::as_str) } diff --git a/crates/paimon/src/table/rest_env.rs b/crates/paimon/src/table/rest_env.rs index b61370b27..4a1f3c778 100644 --- a/crates/paimon/src/table/rest_env.rs +++ b/crates/paimon/src/table/rest_env.rs @@ -106,11 +106,38 @@ impl RESTEnv { data_token_enabled: bool, local_cache: Option>, ) -> Result
{ - let response = api - .get_table(identifier) + let response = Self::fetch_table_response(identifier, &api).await?; + Self::build_table( + identifier, + response, + api, + options, + data_token_enabled, + local_cache, + ) + .await + } + + /// Fetch the raw table metadata, mapping REST errors to catalog errors. + pub(crate) async fn fetch_table_response( + identifier: &Identifier, + api: &RESTApi, + ) -> Result { + api.get_table(identifier) .await - .map_err(|e| map_rest_error_for_table(e, identifier))?; + .map_err(|e| map_rest_error_for_table(e, identifier)) + } + /// Build a Table from an already-fetched response, so type-routed + /// callers can inspect the declared type before constructing. + pub(crate) async fn build_table( + identifier: &Identifier, + response: crate::api::GetTableResponse, + api: Arc, + options: Options, + data_token_enabled: bool, + local_cache: Option>, + ) -> Result
{ let schema = response.schema.ok_or_else(|| Error::DataInvalid { message: format!("Table {} response missing schema", identifier.full_name()), source: None, @@ -128,6 +155,19 @@ impl RESTEnv { ), source: None, })?; + // Fail closed: constructing a non-Paimon table as Paimon would let raw + // `get_table` paths (writes, procedures, time travel) misread it. + if CoreOptions::new(schema.options()).is_non_paimon_table() { + let declared = schema.options().get("type").cloned().unwrap_or_default(); + return Err(Error::Unsupported { + message: format!( + "table '{}' is declared '{declared}' and cannot be read as a Paimon \ + table; only plain reads through a registered table engine are supported", + identifier.full_name() + ), + }); + } + let mut table_schema = TableSchema::new(schema_id, &schema); if CoreOptions::new(table_schema.options()).is_format_table() { table_schema = table_schema.copy_with_options(std::collections::HashMap::from([( diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 2000f9e40..dc2e3a919 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -1501,3 +1501,118 @@ async fn test_catalog_maps_unsupported_view_and_function_endpoints() { paimon::Error::Unsupported { .. } )); } + +#[tokio::test] +async fn test_load_table_routing_returns_non_paimon_for_declared_type() { + use paimon::catalog::RoutedTableLoad; + use std::collections::HashSet; + + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .option("type", "iceberg-table") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "ice_t", schema, "file:///unused"); + + let identifier = Identifier::new("default", "ice_t"); + let non_paimon = HashSet::from(["iceberg-table".to_string()]); + let routed = ctx + .catalog + .load_table_routing(&identifier, &non_paimon) + .await + .unwrap(); + assert!( + matches!(routed, RoutedTableLoad::NonPaimon(ref declared) if declared == "iceberg-table"), + "{routed:?}" + ); + + // Not declared non-Paimon: falls through to Paimon construction. + let routed = ctx + .catalog + .load_table_routing(&identifier, &HashSet::new()) + .await; + assert!(!matches!(routed, Ok(RoutedTableLoad::NonPaimon(_)))); +} + +#[tokio::test] +async fn test_load_table_routing_fails_closed_on_query_auth() { + use std::collections::HashSet; + + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .option("type", "iceberg-table") + .option("query-auth.enabled", "true") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "authed_ice_t", schema, "file:///unused"); + + let identifier = Identifier::new("default", "authed_ice_t"); + let non_paimon = HashSet::from(["iceberg-table".to_string()]); + // Routing must fail closed on query-auth like every Paimon read. + let err = ctx + .catalog + .load_table_routing(&identifier, &non_paimon) + .await + .unwrap_err(); + assert!( + matches!(err, paimon::Error::Unsupported { ref message } if message.contains("query-auth")), + "{err:?}" + ); +} + +#[tokio::test] +async fn test_load_table_routing_constructs_paimon_for_undeclared_type() { + use paimon::catalog::RoutedTableLoad; + use std::collections::HashSet; + + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "plain_t", schema, "file:///unused"); + + let identifier = Identifier::new("default", "plain_t"); + let non_paimon = HashSet::from(["iceberg-table".to_string()]); + let routed = ctx + .catalog + .load_table_routing(&identifier, &non_paimon) + .await + .unwrap(); + match routed { + RoutedTableLoad::Paimon(table) => { + assert_eq!(table.identifier().full_name(), "default.plain_t"); + } + other => panic!("expected a constructed Paimon table, got {other:?}"), + } +} + +#[tokio::test] +async fn test_get_table_fails_closed_on_iceberg_table() { + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .option("type", "iceberg-table") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "raw_ice_t", schema, "file:///unused"); + + // Raw get_table paths fail closed on non-Paimon tables; reads go through + // load_table_routing + an engine. + let err = ctx + .catalog + .get_table(&Identifier::new("default", "raw_ice_t")) + .await + .unwrap_err(); + assert!( + matches!(err, paimon::Error::Unsupported { ref message } + if message.contains("cannot be read as a Paimon table")), + "{err:?}" + ); +} From 9db87f597adc9c0ee0da2232eefd5298d8a36d3e Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Thu, 20 Aug 2026 05:22:01 -0400 Subject: [PATCH 2/6] refactor: route on a typed TableType instead of type strings --- crates/integrations/datafusion/src/catalog.rs | 77 +++++----- .../datafusion/src/sql_context.rs | 7 +- .../datafusion/tests/table_type_routing.rs | 82 ++++------- crates/paimon/src/catalog/mod.rs | 23 ++- .../paimon/src/catalog/rest/rest_catalog.rs | 17 ++- crates/paimon/src/spec/core_options.rs | 34 ++--- crates/paimon/src/spec/mod.rs | 3 + crates/paimon/src/spec/table_type.rs | 134 ++++++++++++++++++ crates/paimon/src/table/rest_env.rs | 12 +- crates/paimon/tests/rest_catalog_test.rs | 80 +++++++++-- 10 files changed, 307 insertions(+), 162 deletions(-) create mode 100644 crates/paimon/src/spec/table_type.rs diff --git a/crates/integrations/datafusion/src/catalog.rs b/crates/integrations/datafusion/src/catalog.rs index 33e4ae662..e3806bc13 100644 --- a/crates/integrations/datafusion/src/catalog.rs +++ b/crates/integrations/datafusion/src/catalog.rs @@ -34,6 +34,7 @@ use datafusion::sql::sqlparser::ast::{Ident, ObjectName, Query, Statement, Visit use datafusion::sql::sqlparser::dialect::GenericDialect; use datafusion::sql::sqlparser::parser::Parser; use paimon::catalog::{Catalog, Identifier, View}; +use paimon::spec::TableType as PaimonTableType; use crate::error::to_datafusion_error; use crate::runtime::{await_with_runtime, block_on_with_runtime}; @@ -45,12 +46,12 @@ pub(crate) type SessionStateProvider = Arc Option + Se /// Engine registry shared between the catalog provider and its schema /// providers, so registrations stay visible to schemas obtained earlier. -type TableEngines = Arc>>>; +type TableEngines = Arc>>>; -/// Resolves tables owned by a non-Paimon engine, selected by the declared -/// `type` table option (see [`PaimonCatalogProvider::register_table_engine`]). -/// `Ok(None)` means not found; errors are propagated verbatim so an engine -/// failure never masquerades as a missing table. +/// Resolves tables owned by another engine (see +/// [`PaimonCatalogProvider::register_table_engine`]). `Ok(None)` means not +/// found; errors propagate, so an engine failure never looks like a missing +/// table. #[async_trait] pub trait TableEngineResolver: Debug + Send + Sync { /// Resolve `database.table` to the engine's table provider. @@ -62,12 +63,11 @@ pub trait TableEngineResolver: Debug + Send + Sync { } /// Read-only wrapper around an engine-resolved provider: reads delegate, -/// DML is rejected. Keeps routed tables on the same fail-closed stance as -/// the raw `get_table` paths even when the engine's provider is writable. +/// DML is rejected even when the engine's own provider is writable. #[derive(Debug)] struct ReadOnlyTableProvider { inner: Arc, - declared: String, + declared: PaimonTableType, table_name: String, } @@ -154,8 +154,8 @@ pub struct PaimonCatalogProvider { blob_reader_registry: BlobReaderRegistry, session_state: Option, schema_force_view_types: bool, - /// Engines for non-Paimon table types, keyed by the declared `type` table - /// option. Same poison-recovery stance as `temp_tables`. + /// Engines for table types served elsewhere, keyed by declared + /// [`PaimonTableType`]. Same poison-recovery stance as `temp_tables`. table_engines: TableEngines, } @@ -196,25 +196,22 @@ impl PaimonCatalogProvider { self } - /// Register an engine for a non-Paimon table type (e.g. `iceberg-table`). - /// References whose metadata declares that type resolve through - /// `resolver`; everything else takes the Paimon path unchanged. Kept - /// inside the provider so the registered catalog type never changes and - /// downcast-based paths (temp tables, time travel) keep working. + /// Register an engine for a table type the Paimon reader cannot serve + /// (e.g. [`PaimonTableType::IcebergTable`]); everything else takes the + /// Paimon path unchanged. Kept inside the provider so the registered + /// catalog type never changes and downcast-based paths (temp tables, + /// time travel) keep working. pub fn register_table_engine( &self, - table_type: impl Into, + table_type: PaimonTableType, resolver: Arc, ) -> DFResult<()> { - // Metadata publishes lowercase TableType values. - let table_type = table_type.into().to_ascii_lowercase(); - // A Paimon-managed type would be split between engines: reads via - // the resolver, raw get_table paths via Paimon. - if !paimon::spec::is_non_paimon_table_type(&table_type) { + // Routing a Paimon-served type would split it between engines: + // reads via the resolver, raw get_table paths via Paimon. + if !table_type.requires_table_engine() { return Err(plan_datafusion_err!( - "table type '{table_type}' is Paimon-managed and cannot be routed to a \ - table engine; routable types: {:?}", - paimon::spec::NON_PAIMON_TABLE_TYPES + "table type '{table_type}' is served by the Paimon reader and cannot be \ + routed to a table engine" )); } self.table_engines @@ -476,7 +473,7 @@ pub struct PaimonSchemaProvider { blob_reader_registry: BlobReaderRegistry, session_state: Option, schema_force_view_types: bool, - /// Engines for non-Paimon table types; empty without routing. + /// Engines for table types served elsewhere; empty without routing. table_engines: TableEngines, } @@ -606,15 +603,15 @@ 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> = self + let table_engines: HashMap> = self .table_engines .read() .unwrap_or_else(|e| e.into_inner()) .clone(); await_with_runtime(async move { - let non_paimon: HashSet = table_engines.keys().cloned().collect(); - match catalog.load_table_routing(&identifier, &non_paimon).await { - Ok(paimon::catalog::RoutedTableLoad::NonPaimon(declared)) => { + let engine_types: HashSet = 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 ('{}')", @@ -695,7 +692,7 @@ impl SchemaProvider for PaimonSchemaProvider { identifier.full_name() ) })?; - validate_view_dependencies(&catalog, &catalog_name, &view, &non_paimon) + validate_view_dependencies(&catalog, &catalog_name, &view, &engine_types) .await?; let mut state = session_state .and_then(|provider| provider()) @@ -779,18 +776,17 @@ impl SchemaProvider for PaimonSchemaProvider { .system_table() .is_some_and(|name| name.eq_ignore_ascii_case("branches")); let has_system_suffix = object.system_table().is_some(); - let engines: HashMap> = self + let engines: HashMap> = self .table_engines .read() .unwrap_or_else(|e| e.into_inner()) .clone(); block_on_with_runtime( async move { - let non_paimon: HashSet = engines.keys().cloned().collect(); - match catalog.load_table_routing(&identifier, &non_paimon).await { - Ok(paimon::catalog::RoutedTableLoad::NonPaimon(declared)) => { - // Branches and system tables are Paimon-only; - // `table()` rejects them for routed tables. + let engine_types: HashSet = engines.keys().copied().collect(); + match catalog.load_table_routing(&identifier, &engine_types).await { + Ok(paimon::catalog::RoutedTableLoad::Engine(declared)) => { + // Paimon-only; `table()` rejects them here too. if branch.is_some() || has_system_suffix { return false; } @@ -916,7 +912,7 @@ async fn validate_view_dependencies( catalog: &Arc, catalog_name: &str, root: &View, - non_paimon_types: &HashSet, + engine_types: &HashSet, ) -> DFResult<()> { let mut queue = VecDeque::from([root.clone()]); let mut loaded = HashSet::from([root.identifier().clone()]); @@ -926,11 +922,8 @@ async fn validate_view_dependencies( let candidates = view_relation_identifiers(&view, catalog_name)?; let mut view_dependencies = Vec::new(); for identifier in candidates { - // Routed non-Paimon tables count as existing dependencies. - match catalog - .load_table_routing(&identifier, non_paimon_types) - .await - { + // Routed engine tables count as existing dependencies. + match catalog.load_table_routing(&identifier, engine_types).await { Ok(_) => continue, Err(paimon::Error::TableNotExist { .. }) | Err(paimon::Error::Unsupported { .. }) => {} diff --git a/crates/integrations/datafusion/src/sql_context.rs b/crates/integrations/datafusion/src/sql_context.rs index 06d215250..bb30c7348 100644 --- a/crates/integrations/datafusion/src/sql_context.rs +++ b/crates/integrations/datafusion/src/sql_context.rs @@ -353,13 +353,14 @@ impl SQLContext { self.catalogs.contains_key(name) } - /// Register an engine for a non-Paimon table type (the declared `type` - /// table option, e.g. `iceberg-table`) on a registered Paimon catalog. + /// Register an engine for a table type served by another engine (e.g. + /// [`paimon::spec::TableType::IcebergTable`]) on a registered Paimon + /// catalog. /// See [`crate::catalog::PaimonCatalogProvider::register_table_engine`]. pub fn register_catalog_table_engine( &self, catalog_name: &str, - table_type: &str, + table_type: paimon::spec::TableType, resolver: Arc, ) -> DFResult<()> { let catalog_provider = self diff --git a/crates/integrations/datafusion/tests/table_type_routing.rs b/crates/integrations/datafusion/tests/table_type_routing.rs index f9d2a77c1..257b2bde7 100644 --- a/crates/integrations/datafusion/tests/table_type_routing.rs +++ b/crates/integrations/datafusion/tests/table_type_routing.rs @@ -29,7 +29,7 @@ use datafusion::arrow::record_batch::RecordBatch; use datafusion::datasource::{MemTable, TableProvider}; use datafusion::error::{DataFusionError, Result as DFResult}; use paimon::catalog::{Catalog, Database, Identifier, RoutedTableLoad}; -use paimon::spec::{Schema as PaimonSchema, SchemaChange}; +use paimon::spec::{Schema as PaimonSchema, SchemaChange, TableType}; use paimon::table::Table; use paimon::{CatalogOptions, FileSystemCatalog, Options, Result as PaimonResult}; use paimon_datafusion::{SQLContext, TableEngineResolver}; @@ -37,14 +37,13 @@ use tempfile::TempDir; const CATALOG: &str = "cat"; const DB: &str = "shared_db"; -const ICEBERG_TABLE_TYPE: &str = "iceberg-table"; /// A filesystem-backed catalog that declares table types the way a REST /// catalog does through the `type` table option. #[derive(Debug)] struct TypedTestCatalog { inner: Arc, - declared_types: HashMap, + declared_types: HashMap, } #[async_trait] @@ -80,7 +79,7 @@ impl Catalog for TypedTestCatalog { } async fn get_table(&self, identifier: &Identifier) -> PaimonResult
{ - // Mirror the REST catalog: declared-non-Paimon tables fail closed. + // Mirror the REST catalog: engine-served tables fail closed. if let Some(declared) = self.declared_types.get(identifier.object()) { return Err(paimon::Error::Unsupported { message: format!( @@ -95,12 +94,11 @@ impl Catalog for TypedTestCatalog { async fn load_table_routing( &self, identifier: &Identifier, - non_paimon_types: &HashSet, + engine_types: &HashSet, ) -> PaimonResult { if let Some(declared) = self.declared_types.get(identifier.object()) { - let declared = declared.to_ascii_lowercase(); - if non_paimon_types.contains(&declared) { - return Ok(RoutedTableLoad::NonPaimon(declared)); + if engine_types.contains(declared) { + return Ok(RoutedTableLoad::Engine(*declared)); } } Ok(RoutedTableLoad::Paimon(Box::new( @@ -207,11 +205,11 @@ async fn setup() -> TestEnv { let typed_catalog = Arc::new(TypedTestCatalog { inner: fs_catalog, declared_types: HashMap::from([ - ("it".to_string(), ICEBERG_TABLE_TYPE.to_string()), + ("it".to_string(), TableType::IcebergTable), // Unknown to the engine: existence mirrors the resolver's miss. - ("ghost".to_string(), ICEBERG_TABLE_TYPE.to_string()), + ("ghost".to_string(), TableType::IcebergTable), // Write-statement target: raw get_table paths fail closed. - ("ft".to_string(), ICEBERG_TABLE_TYPE.to_string()), + ("ft".to_string(), TableType::IcebergTable), ]), }); let mut ctx = SQLContext::new(); @@ -232,8 +230,12 @@ async fn setup() -> TestEnv { ctx.sql(&stmt).await.unwrap().collect().await.unwrap(); } - ctx.register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(FakeEngineResolver)) - .unwrap(); + ctx.register_catalog_table_engine( + CATALOG, + TableType::IcebergTable, + Arc::new(FakeEngineResolver), + ) + .unwrap(); TestEnv { _paimon_dir: paimon_dir, @@ -266,7 +268,7 @@ async fn paimon_path_still_serves_paimon_tables() { } #[tokio::test] -async fn declared_non_paimon_table_routes_to_engine() { +async fn declared_engine_table_routes_to_engine() { let env = setup().await; let df = env .ctx @@ -406,7 +408,7 @@ impl TableEngineResolver for BrokenResolver { async fn engine_errors_are_surfaced() { let env = setup().await; env.ctx - .register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(BrokenResolver)) + .register_catalog_table_engine(CATALOG, TableType::IcebergTable, Arc::new(BrokenResolver)) .unwrap(); let Err(err) = env .ctx @@ -419,7 +421,7 @@ async fn engine_errors_are_surfaced() { assert!(msg.contains("engine backend exploded"), "{msg}"); } -// A declared non-Paimon type with no registered engine takes the Paimon path. +// A declared engine type with no registered engine takes the Paimon path. #[tokio::test] async fn undeclared_engine_type_takes_paimon_path() { let paimon_dir = TempDir::new().unwrap(); @@ -429,7 +431,7 @@ async fn undeclared_engine_type_takes_paimon_path() { let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); let typed_catalog = Arc::new(TypedTestCatalog { inner: fs_catalog, - declared_types: HashMap::from([("ot".to_string(), "object-table".to_string())]), + declared_types: HashMap::from([("ot".to_string(), TableType::ObjectTable)]), }); let mut ctx = SQLContext::new(); ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); @@ -458,7 +460,7 @@ async fn writes_to_routed_tables_fail_closed() { }; let msg = err.to_string(); assert!(msg.contains("cannot be read as a Paimon table"), "{msg}"); - assert!(msg.contains(ICEBERG_TABLE_TYPE), "{msg}"); + assert!(msg.contains(TableType::IcebergTable.as_str()), "{msg}"); } // System tables are Paimon-only; routed tables get a clear error. @@ -487,49 +489,19 @@ async fn table_exist_mirrors_the_resolver() { assert!(!schema.table_exist("it$snapshots")); } -// Only declared non-Paimon table types may be routed. +// Only table types the Paimon reader cannot serve may be routed. #[tokio::test] async fn paimon_managed_types_cannot_be_routed() { let env = setup().await; - let Err(err) = - env.ctx - .register_catalog_table_engine(CATALOG, "lance-table", Arc::new(FakeEngineResolver)) - else { + let Err(err) = env.ctx.register_catalog_table_engine( + CATALOG, + TableType::LanceTable, + Arc::new(FakeEngineResolver), + ) else { panic!("registering an engine for a Paimon-managed type must fail"); }; let msg = err.to_string(); - assert!(msg.contains("Paimon-managed"), "{msg}"); -} - -// Registration canonicalizes the type key. -#[tokio::test] -async fn mixed_case_registration_still_routes() { - let env = setup().await; - env.ctx - .register_catalog_table_engine(CATALOG, "ICEBERG-TABLE", Arc::new(FakeEngineResolver)) - .unwrap(); - let batches = env - .ctx - .sql(&format!("SELECT id FROM {CATALOG}.{DB}.it ORDER BY id")) - .await - .unwrap() - .collect() - .await - .unwrap(); - assert_eq!(column_i32(&batches), vec![1, 3]); -} - -// Resolver failures must not masquerade as misses in existence checks. -#[tokio::test] -async fn table_exist_preserves_resolver_failures() { - let env = setup().await; - env.ctx - .register_catalog_table_engine(CATALOG, ICEBERG_TABLE_TYPE, Arc::new(BrokenResolver)) - .unwrap(); - let provider = env.ctx.ctx().catalog(CATALOG).unwrap(); - let schema = provider.schema(DB).unwrap(); - // Conservatively "exists"; the follow-up table() surfaces the error. - assert!(schema.table_exist("it")); + assert!(msg.contains("served by the Paimon reader"), "{msg}"); } // DML against a routed table is rejected even when the engine's provider diff --git a/crates/paimon/src/catalog/mod.rs b/crates/paimon/src/catalog/mod.rs index 0aadaef67..e20b1ff38 100644 --- a/crates/paimon/src/catalog/mod.rs +++ b/crates/paimon/src/catalog/mod.rs @@ -262,18 +262,16 @@ impl fmt::Debug for Identifier { use async_trait::async_trait; use crate::api::PagedList; -use crate::spec::{Partition, Schema, SchemaChange}; +use crate::spec::{Partition, Schema, SchemaChange, TableType}; use crate::table::Table; /// Outcome of [`Catalog::load_table_routing`]. #[derive(Debug)] pub enum RoutedTableLoad { - /// The table was constructed as a Paimon table (boxed: `Table` is much - /// larger than the other variant). + /// A constructed Paimon table (boxed: far larger than the other variant). Paimon(Box
), - /// The declared table type matched `non_paimon_types`; construction was - /// skipped. - NonPaimon(String), + /// The declared type is in `engine_types`; construction was skipped. + Engine(TableType), } /// Catalog API for reading and writing metadata (databases, tables) in Paimon. @@ -334,20 +332,19 @@ pub trait Catalog: Send + Sync { /// * [`crate::Error::TableNotExist`] - table does not exist. async fn get_table(&self, identifier: &Identifier) -> Result
; - /// Load a table, or return only its declared type (the `type` table - /// option) when it matches one of `non_paimon_types` — skipping - /// construction and any token/FileIO I/O. One metadata round-trip - /// either way. The default implementation always constructs, for - /// catalogs without a table-type concept. + /// Load a table, or return only its declared [`TableType`] when that + /// type is in `engine_types`, skipping construction and its token/FileIO + /// I/O. One metadata round-trip either way. The default implementation + /// always constructs, for catalogs without a table-type concept. /// /// # Errors /// Same as [`Catalog::get_table`]. async fn load_table_routing( &self, identifier: &Identifier, - non_paimon_types: &std::collections::HashSet, + engine_types: &std::collections::HashSet, ) -> Result { - let _ = non_paimon_types; + let _ = engine_types; Ok(RoutedTableLoad::Paimon(Box::new( self.get_table(identifier).await?, ))) diff --git a/crates/paimon/src/catalog/rest/rest_catalog.rs b/crates/paimon/src/catalog/rest/rest_catalog.rs index 662a8650c..a15a53777 100644 --- a/crates/paimon/src/catalog/rest/rest_catalog.rs +++ b/crates/paimon/src/catalog/rest/rest_catalog.rs @@ -218,18 +218,17 @@ impl Catalog for RESTCatalog { async fn load_table_routing( &self, identifier: &Identifier, - non_paimon_types: &std::collections::HashSet, + engine_types: &std::collections::HashSet, ) -> Result { let response = RESTEnv::fetch_table_response(identifier, &self.api).await?; if let Some(schema) = response.schema.as_ref() { - if let Some(declared) = schema.options().get("type") { - let declared = declared.to_ascii_lowercase(); - if non_paimon_types.contains(&declared) { - // Neither this client nor an engine resolver can enforce - // the server-side row filter / column masking. - crate::spec::CoreOptions::new(schema.options()).ensure_read_authorized()?; - return Ok(crate::catalog::RoutedTableLoad::NonPaimon(declared)); - } + let options = crate::spec::CoreOptions::new(schema.options()); + let declared = options.table_type()?; + if engine_types.contains(&declared) { + // Neither this client nor an engine can enforce the + // server-side row filter / column mask. + options.ensure_read_authorized()?; + return Ok(crate::catalog::RoutedTableLoad::Engine(declared)); } } RESTEnv::build_table( diff --git a/crates/paimon/src/spec/core_options.rs b/crates/paimon/src/spec/core_options.rs index 606af71c0..0acab7371 100644 --- a/crates/paimon/src/spec/core_options.rs +++ b/crates/paimon/src/spec/core_options.rs @@ -17,6 +17,8 @@ use std::collections::{HashMap, HashSet}; +use crate::spec::TableType; + const DELETION_VECTORS_ENABLED_OPTION: &str = "deletion-vectors.enabled"; const DELETION_VECTORS_MERGE_ON_READ_OPTION: &str = "deletion-vectors.merge-on-read"; pub(crate) const QUERY_AUTH_ENABLED_OPTION: &str = "query-auth.enabled"; @@ -74,18 +76,6 @@ const ROW_TRACKING_ENABLED_OPTION: &str = "row-tracking.enabled"; const CLUSTERING_INCREMENTAL_OPTION: &str = "clustering.incremental"; pub(crate) const TABLE_TYPE_OPTION: &str = "type"; -/// Declared table types a Paimon reader cannot serve; the only types -/// accepted for table-engine routing. The Java side loads these as -/// metadata-only tables (e.g. `IcebergTable`). -pub const NON_PAIMON_TABLE_TYPES: &[&str] = &["iceberg-table"]; - -/// Whether `table_type` is one of [`NON_PAIMON_TABLE_TYPES`]. -pub fn is_non_paimon_table_type(table_type: &str) -> bool { - NON_PAIMON_TABLE_TYPES - .iter() - .any(|declared| table_type.eq_ignore_ascii_case(declared)) -} -pub(crate) const FORMAT_TABLE_TYPE: &str = "format-table"; pub(crate) const PATH_OPTION: &str = "path"; const MANIFEST_COMPRESSION_OPTION: &str = "manifest.compression"; const MANIFEST_TARGET_FILE_SIZE_OPTION: &str = "manifest.target-file-size"; @@ -636,19 +626,17 @@ impl<'a> CoreOptions<'a> { .unwrap_or(false) } - pub fn is_format_table(&self) -> bool { - self.options - .get(TABLE_TYPE_OPTION) - .map(|value| value.eq_ignore_ascii_case(FORMAT_TABLE_TYPE)) - .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 { + match self.options.get(TABLE_TYPE_OPTION) { + Some(value) => value.parse(), + None => Ok(TableType::default()), + } } - /// Whether the declared table type is one of [`NON_PAIMON_TABLE_TYPES`]. - pub fn is_non_paimon_table(&self) -> bool { - self.options - .get(TABLE_TYPE_OPTION) - .map(|value| is_non_paimon_table_type(value)) - .unwrap_or(false) + pub fn is_format_table(&self) -> bool { + matches!(self.table_type(), Ok(TableType::FormatTable)) } pub fn path(&self) -> Option<&str> { diff --git a/crates/paimon/src/spec/mod.rs b/crates/paimon/src/spec/mod.rs index 3fb3802e9..a2613fecf 100644 --- a/crates/paimon/src/spec/mod.rs +++ b/crates/paimon/src/spec/mod.rs @@ -31,6 +31,9 @@ pub use blob_view_struct::BlobViewStruct; mod data_file; pub use data_file::*; +mod table_type; +pub use table_type::*; + mod core_options; pub(crate) use core_options::TimeTravelSelector; pub use core_options::*; diff --git a/crates/paimon/src/spec/table_type.rs b/crates/paimon/src/spec/table_type.rs new file mode 100644 index 000000000..40c147320 --- /dev/null +++ b/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 . + LanceTable, + /// An iceberg table, see . + 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) + } +} + +impl Display for TableType { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +impl FromStr for TableType { + type Err = Error; + + fn from_str(value: &str) -> Result { + [ + TableType::Table, + TableType::FormatTable, + TableType::MaterializedTable, + TableType::ObjectTable, + TableType::LanceTable, + TableType::IcebergTable, + ] + .into_iter() + .find(|table_type| value.eq_ignore_ascii_case(table_type.as_str())) + .ok_or_else(|| Error::Unsupported { + message: format!("unknown table type: {value}"), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_every_declared_value() { + for table_type in [ + TableType::Table, + TableType::FormatTable, + TableType::MaterializedTable, + TableType::ObjectTable, + TableType::LanceTable, + TableType::IcebergTable, + ] { + assert_eq!( + TableType::from_str(table_type.as_str()).unwrap(), + table_type + ); + assert_eq!( + TableType::from_str(&table_type.as_str().to_uppercase()).unwrap(), + table_type + ); + } + } + + #[test] + fn rejects_unknown_value() { + let err = TableType::from_str("delta-table").expect_err("unknown type must not parse"); + assert!(err.to_string().contains("delta-table"), "{err}"); + } + + #[test] + fn only_iceberg_requires_an_engine() { + assert!(TableType::IcebergTable.requires_table_engine()); + for table_type in [ + TableType::Table, + TableType::FormatTable, + TableType::MaterializedTable, + TableType::ObjectTable, + TableType::LanceTable, + ] { + assert!(!table_type.requires_table_engine(), "{table_type}"); + } + } +} diff --git a/crates/paimon/src/table/rest_env.rs b/crates/paimon/src/table/rest_env.rs index 4a1f3c778..7d716cb7a 100644 --- a/crates/paimon/src/table/rest_env.rs +++ b/crates/paimon/src/table/rest_env.rs @@ -128,8 +128,8 @@ impl RESTEnv { .map_err(|e| map_rest_error_for_table(e, identifier)) } - /// Build a Table from an already-fetched response, so type-routed - /// callers can inspect the declared type before constructing. + /// Build a Table from an already-fetched response, so routing can + /// inspect the declared type first. pub(crate) async fn build_table( identifier: &Identifier, response: crate::api::GetTableResponse, @@ -155,10 +155,10 @@ impl RESTEnv { ), source: None, })?; - // Fail closed: constructing a non-Paimon table as Paimon would let raw - // `get_table` paths (writes, procedures, time travel) misread it. - if CoreOptions::new(schema.options()).is_non_paimon_table() { - let declared = schema.options().get("type").cloned().unwrap_or_default(); + // Fail closed: constructed as Paimon, raw `get_table` paths (writes, + // procedures, time travel) would misread it. + let declared = CoreOptions::new(schema.options()).table_type()?; + if declared.requires_table_engine() { return Err(Error::Unsupported { message: format!( "table '{}' is declared '{declared}' and cannot be read as a Paimon \ diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index dc2e3a919..4fede10d6 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -1503,8 +1503,9 @@ async fn test_catalog_maps_unsupported_view_and_function_endpoints() { } #[tokio::test] -async fn test_load_table_routing_returns_non_paimon_for_declared_type() { +async fn test_load_table_routing_returns_engine_for_declared_type() { use paimon::catalog::RoutedTableLoad; + use paimon::spec::TableType; use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; @@ -1517,27 +1518,28 @@ async fn test_load_table_routing_returns_non_paimon_for_declared_type() { .add_table_with_schema("default", "ice_t", schema, "file:///unused"); let identifier = Identifier::new("default", "ice_t"); - let non_paimon = HashSet::from(["iceberg-table".to_string()]); + let engine_types = HashSet::from([TableType::IcebergTable]); let routed = ctx .catalog - .load_table_routing(&identifier, &non_paimon) + .load_table_routing(&identifier, &engine_types) .await .unwrap(); assert!( - matches!(routed, RoutedTableLoad::NonPaimon(ref declared) if declared == "iceberg-table"), + matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), "{routed:?}" ); - // Not declared non-Paimon: falls through to Paimon construction. + // No engine registered for the type: falls through to Paimon construction. let routed = ctx .catalog .load_table_routing(&identifier, &HashSet::new()) .await; - assert!(!matches!(routed, Ok(RoutedTableLoad::NonPaimon(_)))); + assert!(!matches!(routed, Ok(RoutedTableLoad::Engine(_)))); } #[tokio::test] async fn test_load_table_routing_fails_closed_on_query_auth() { + use paimon::spec::TableType; use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; @@ -1551,11 +1553,11 @@ async fn test_load_table_routing_fails_closed_on_query_auth() { .add_table_with_schema("default", "authed_ice_t", schema, "file:///unused"); let identifier = Identifier::new("default", "authed_ice_t"); - let non_paimon = HashSet::from(["iceberg-table".to_string()]); + let engine_types = HashSet::from([TableType::IcebergTable]); // Routing must fail closed on query-auth like every Paimon read. let err = ctx .catalog - .load_table_routing(&identifier, &non_paimon) + .load_table_routing(&identifier, &engine_types) .await .unwrap_err(); assert!( @@ -1567,6 +1569,7 @@ async fn test_load_table_routing_fails_closed_on_query_auth() { #[tokio::test] async fn test_load_table_routing_constructs_paimon_for_undeclared_type() { use paimon::catalog::RoutedTableLoad; + use paimon::spec::TableType; use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; @@ -1578,10 +1581,10 @@ async fn test_load_table_routing_constructs_paimon_for_undeclared_type() { .add_table_with_schema("default", "plain_t", schema, "file:///unused"); let identifier = Identifier::new("default", "plain_t"); - let non_paimon = HashSet::from(["iceberg-table".to_string()]); + let engine_types = HashSet::from([TableType::IcebergTable]); let routed = ctx .catalog - .load_table_routing(&identifier, &non_paimon) + .load_table_routing(&identifier, &engine_types) .await .unwrap(); match routed { @@ -1603,7 +1606,7 @@ async fn test_get_table_fails_closed_on_iceberg_table() { ctx.server .add_table_with_schema("default", "raw_ice_t", schema, "file:///unused"); - // Raw get_table paths fail closed on non-Paimon tables; reads go through + // Raw get_table paths fail closed on engine-served tables; reads go through // load_table_routing + an engine. let err = ctx .catalog @@ -1616,3 +1619,58 @@ async fn test_get_table_fails_closed_on_iceberg_table() { "{err:?}" ); } + +#[tokio::test] +async fn test_load_table_routing_parses_declared_type_case_insensitively() { + use paimon::catalog::RoutedTableLoad; + use paimon::spec::TableType; + use std::collections::HashSet; + + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .option("type", "ICEBERG-TABLE") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "upper_ice_t", schema, "file:///unused"); + + let routed = ctx + .catalog + .load_table_routing( + &Identifier::new("default", "upper_ice_t"), + &HashSet::from([TableType::IcebergTable]), + ) + .await + .unwrap(); + assert!( + matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + "{routed:?}" + ); +} + +#[tokio::test] +async fn test_load_table_routing_rejects_unknown_declared_type() { + use std::collections::HashSet; + + let ctx = setup_catalog(vec!["default"]).await; + let schema = Schema::builder() + .column("id", DataType::BigInt(BigIntType::new())) + .option("type", "delta-table") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "delta_t", schema, "file:///unused"); + + // A type this client does not know is not silently read as Paimon. + let err = ctx + .catalog + .load_table_routing(&Identifier::new("default", "delta_t"), &HashSet::new()) + .await + .unwrap_err(); + assert!( + matches!(err, paimon::Error::Unsupported { ref message } + if message.contains("unknown table type")), + "{err:?}" + ); +} From 8d7026af1bf397b6425d8aa3a53f33332685e236 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Thu, 20 Aug 2026 23:08:15 -0400 Subject: [PATCH 3/6] fix: route declared table types in the filesystem catalog --- crates/paimon-rest-server/src/lib.rs | 11 +- crates/paimon-rest-server/tests/e2e.rs | 47 ++++++++ crates/paimon/src/catalog/filesystem.rs | 150 ++++++++++++++++++++---- 3 files changed, 180 insertions(+), 28 deletions(-) diff --git a/crates/paimon-rest-server/src/lib.rs b/crates/paimon-rest-server/src/lib.rs index 434f38de3..10d0efc28 100644 --- a/crates/paimon-rest-server/src/lib.rs +++ b/crates/paimon-rest-server/src/lib.rs @@ -416,12 +416,13 @@ async fn create_table( async fn get_table(path: RestPath, Extension(state): Extension>) -> Response { let table = path.get("table"); let identifier = Identifier::new(path.get("db"), table.clone()); - let resolved = match state.catalog.get_table(&identifier).await { - Ok(t) => t, + // Raw metadata, not a constructed table: engine-served types (e.g. + // `iceberg-table`) must still be describable so clients can route them. + let (location, loaded_schema) = match state.catalog.fetch_table_schema(&identifier).await { + Ok(loaded) => loaded, Err(e) => return error_response(e), }; - - let table_schema = resolved.schema(); + let table_schema = &loaded_schema; // Convert the stored `TableSchema` into the DDL `Schema` the response // carries. `Schema` is a field subset of `TableSchema` (both camelCase), // and serde ignores the extra keys, preserving field ids exactly. @@ -441,7 +442,7 @@ async fn get_table(path: RestPath, Extension(state): Extension>) - // that satisfies the client's RESTEnv requirement. Some(identifier.full_name()), Some(table), - Some(resolved.location().to_string()), + Some(location), Some(false), Some(table_schema.id()), Some(schema), diff --git a/crates/paimon-rest-server/tests/e2e.rs b/crates/paimon-rest-server/tests/e2e.rs index 6edf274b3..fc8454532 100644 --- a/crates/paimon-rest-server/tests/e2e.rs +++ b/crates/paimon-rest-server/tests/e2e.rs @@ -429,3 +429,50 @@ async fn test_special_char_names() { cat.drop_database(plus_db, false, false).await.unwrap(); assert!(cat.list_databases().await.unwrap().is_empty()); } + +/// A table declared as an engine-served type stays describable over REST, so +/// the client can route it instead of reading it as Paimon. +#[tokio::test] +async fn declared_engine_type_routes_through_the_rest_catalog() { + use paimon::catalog::RoutedTableLoad; + use paimon::spec::TableType; + use std::collections::HashSet; + + let ctx = setup().await; + ctx.catalog + .create_database("db", false, HashMap::new()) + .await + .expect("create database"); + + let schema = Schema::builder() + .column("id", DataType::Int(IntType::new())) + .option("type", "iceberg-table") + .build() + .expect("build schema"); + let identifier = Identifier::new("db", "ice_t"); + ctx.catalog + .create_table(&identifier, schema, false) + .await + .expect("create table"); + + let routed = ctx + .catalog + .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) + .await + .expect("routing must reach the declared type"); + assert!( + matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + "{routed:?}" + ); + + // Raw get_table still fails closed on both sides of the wire. + let err = ctx + .catalog + .get_table(&identifier) + .await + .expect_err("an engine-served table must not construct as Paimon"); + assert!( + err.to_string().contains("cannot be read as a Paimon table"), + "{err}" + ); +} diff --git a/crates/paimon/src/catalog/filesystem.rs b/crates/paimon/src/catalog/filesystem.rs index ae015e254..dab90d544 100644 --- a/crates/paimon/src/catalog/filesystem.rs +++ b/crates/paimon/src/catalog/filesystem.rs @@ -26,7 +26,7 @@ use crate::common::{CatalogOptions, Options}; use crate::error::{ConfigInvalidSnafu, Error, Result}; use crate::io::cache::create_local_cache; use crate::io::FileIO; -use crate::spec::{Schema, TableSchema}; +use crate::spec::{CoreOptions, Schema, TableSchema, TableType}; use crate::table::{SchemaManager, Table}; use async_trait::async_trait; use bytes::Bytes; @@ -170,6 +170,63 @@ impl FileSystemCatalog { Ok(dirs) } + /// Fetch the stored path and schema of an existing table, bypassing the + /// engine-type guard in [`Self::build_table`]. Routing inspects the + /// declared type through this, and a catalog server must keep serving raw + /// metadata for engine-served types so its clients can route them. + pub async fn fetch_table_schema( + &self, + identifier: &Identifier, + ) -> Result<(String, TableSchema)> { + identifier.validate()?; + + let table_path = self.table_path(identifier); + + if !self.table_exists(identifier).await? { + return Err(Error::TableNotExist { + full_name: identifier.full_name(), + }); + } + + let schema = self + .load_latest_table_schema(&table_path) + .await? + .ok_or_else(|| Error::TableNotExist { + full_name: identifier.full_name(), + })?; + + Ok((table_path, schema)) + } + + /// Build a Table from an already-fetched schema. + fn build_table( + &self, + identifier: &Identifier, + table_path: String, + schema: TableSchema, + ) -> Result
{ + // Fail closed: constructed as Paimon, raw `get_table` paths (writes, + // procedures, time travel) would misread it. + let declared = CoreOptions::new(schema.options()).table_type()?; + if declared.requires_table_engine() { + return Err(Error::Unsupported { + message: format!( + "table '{}' is declared '{declared}' and cannot be read as a Paimon \ + table; only plain reads through a registered table engine are supported", + identifier.full_name() + ), + }); + } + + Ok(Table::new( + self.file_io.clone(), + identifier.clone(), + table_path, + schema, + None, + )) + } + /// Load the latest schema for a table (highest schema-{version} file under table_path/schema). async fn load_latest_table_schema(&self, table_path: &str) -> Result> { let manager = SchemaManager::new(self.file_io.clone(), table_path.to_string()); @@ -297,30 +354,26 @@ impl Catalog for FileSystemCatalog { } async fn get_table(&self, identifier: &Identifier) -> Result
{ - 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, + ) -> Result { + let (table_path, schema) = self.fetch_table_schema(identifier).await?; + let options = CoreOptions::new(schema.options()); + let declared = options.table_type()?; + if engine_types.contains(&declared) { + // Neither this client nor an engine can enforce the + // server-side row filter / column mask. + options.ensure_read_authorized()?; + return Ok(crate::catalog::RoutedTableLoad::Engine(declared)); } - - let schema = self - .load_latest_table_schema(&table_path) - .await? - .ok_or_else(|| Error::TableNotExist { - full_name: identifier.full_name(), - })?; - - Ok(Table::new( - self.file_io.clone(), - identifier.clone(), - table_path, - schema, - None, - )) + self.build_table(identifier, table_path, schema) + .map(|table| crate::catalog::RoutedTableLoad::Paimon(Box::new(table))) } async fn list_tables(&self, database_name: &str) -> Result> { @@ -665,6 +718,57 @@ mod tests { assert!(escaped_path.exists()); } + #[tokio::test] + async fn test_declared_engine_type_routes_and_fails_closed() { + use crate::catalog::RoutedTableLoad; + use std::collections::HashSet; + + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + let schema = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", "iceberg-table") + .build() + .unwrap(); + let identifier = Identifier::new("db1", "ice_t"); + catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + + // Routed to the engine, exactly like the REST catalog does. + let routed = catalog + .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) + .await + .unwrap(); + assert!( + matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + "{routed:?}" + ); + + // Raw get_table fails closed, so writes cannot reach a Paimon writer. + let err = catalog.get_table(&identifier).await.unwrap_err(); + assert!( + matches!(err, Error::Unsupported { ref message } + if message.contains("cannot be read as a Paimon table")), + "{err:?}" + ); + + // No engine registered for the type: still fails closed rather than + // silently reading it as Paimon. + let err = catalog + .load_table_routing(&identifier, &HashSet::new()) + .await + .unwrap_err(); + assert!(matches!(err, Error::Unsupported { .. }), "{err:?}"); + } + #[tokio::test] async fn test_table_operations() { let (_temp_dir, catalog) = create_test_catalog(); From 69ed0009e1a2f2d8be34359b91b0149e73710e07 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 21 Aug 2026 02:46:46 -0400 Subject: [PATCH 4/6] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20engin?= =?UTF-8?q?e=20types,=20time=20travel,=20immutable=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/integrations/datafusion/src/catalog.rs | 23 ++- .../datafusion/src/relation_planner.rs | 10 ++ .../datafusion/tests/table_type_routing.rs | 168 ++++++++++++++---- crates/paimon-rest-server/src/lib.rs | 4 +- crates/paimon-rest-server/tests/e2e.rs | 56 +++++- crates/paimon/src/catalog/filesystem.rs | 157 +++++++++++++++- crates/paimon/src/spec/core_options.rs | 7 + crates/paimon/src/spec/schema.rs | 3 + crates/paimon/src/spec/table_type.rs | 23 ++- crates/paimon/tests/rest_catalog_test.rs | 11 +- 10 files changed, 396 insertions(+), 66 deletions(-) diff --git a/crates/integrations/datafusion/src/catalog.rs b/crates/integrations/datafusion/src/catalog.rs index e3806bc13..defdc070c 100644 --- a/crates/integrations/datafusion/src/catalog.rs +++ b/crates/integrations/datafusion/src/catalog.rs @@ -65,10 +65,10 @@ pub trait TableEngineResolver: Debug + Send + Sync { /// Read-only wrapper around an engine-resolved provider: reads delegate, /// DML is rejected even when the engine's own provider is writable. #[derive(Debug)] -struct ReadOnlyTableProvider { +pub(crate) struct ReadOnlyTableProvider { inner: Arc, - declared: PaimonTableType, - table_name: String, + pub(crate) declared: PaimonTableType, + pub(crate) table_name: String, } #[async_trait] @@ -619,6 +619,23 @@ impl SchemaProvider for PaimonSchemaProvider { identifier.full_name() )); } + // The Paimon arm below applies these; an engine would + // ignore them and answer from current data. + let session_options = dynamic_options + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone(); + let session_options = paimon::spec::CoreOptions::new(&session_options); + session_options + .validate_scan_options() + .map_err(to_datafusion_error)?; + if session_options.has_time_travel_selector() { + return Err(plan_datafusion_err!( + "time travel is not supported for routed '{}' tables ('{}')", + declared, + identifier.full_name() + )); + } let resolver = table_engines .get(&declared) .expect("declared type came from this engine map"); diff --git a/crates/integrations/datafusion/src/relation_planner.rs b/crates/integrations/datafusion/src/relation_planner.rs index e51bdcd10..2dae8d0fb 100644 --- a/crates/integrations/datafusion/src/relation_planner.rs +++ b/crates/integrations/datafusion/src/relation_planner.rs @@ -22,6 +22,7 @@ use std::fmt::Debug; use std::sync::Arc; use datafusion::catalog::default_table_source::{provider_as_source, source_as_provider}; +use datafusion::common::plan_datafusion_err; use datafusion::common::TableReference; use datafusion::error::Result as DFResult; use datafusion::logical_expr::builder::LogicalPlanBuilder; @@ -31,6 +32,7 @@ use datafusion::logical_expr::planner::{ use datafusion::sql::sqlparser::ast::{self, TableFactor, TableVersion}; use paimon::spec::{SCAN_TIMESTAMP_MILLIS_OPTION, SCAN_VERSION_OPTION}; +use crate::catalog::ReadOnlyTableProvider; use crate::table::PaimonTableProvider; /// A [`RelationPlanner`] that intercepts `VERSION AS OF` and `TIMESTAMP AS OF` @@ -85,6 +87,14 @@ impl RelationPlanner for PaimonRelationPlanner { // Check if this is a Paimon table. let Some(paimon_provider) = provider.downcast_ref::() else { + if let Some(routed) = provider.downcast_ref::() { + // Falling through drops the version clause silently. + return Err(plan_datafusion_err!( + "time travel is not supported for routed '{}' tables ('{}')", + routed.declared, + routed.table_name + )); + } return Ok(RelationPlanning::Original(Box::new(relation))); }; diff --git a/crates/integrations/datafusion/tests/table_type_routing.rs b/crates/integrations/datafusion/tests/table_type_routing.rs index 257b2bde7..34a31df80 100644 --- a/crates/integrations/datafusion/tests/table_type_routing.rs +++ b/crates/integrations/datafusion/tests/table_type_routing.rs @@ -15,10 +15,6 @@ // specific language governing permissions and limitations // under the License. -//! Declared-type table routing: tables declared as a non-Paimon type (e.g. -//! `iceberg-table`) resolve through the registered engine; everything else -//! takes the Paimon path unchanged. - use std::collections::{HashMap, HashSet}; use std::sync::Arc; @@ -38,8 +34,6 @@ use tempfile::TempDir; const CATALOG: &str = "cat"; const DB: &str = "shared_db"; -/// A filesystem-backed catalog that declares table types the way a REST -/// catalog does through the `type` table option. #[derive(Debug)] struct TypedTestCatalog { inner: Arc, @@ -79,7 +73,6 @@ impl Catalog for TypedTestCatalog { } async fn get_table(&self, identifier: &Identifier) -> PaimonResult
{ - // Mirror the REST catalog: engine-served tables fail closed. if let Some(declared) = self.declared_types.get(identifier.object()) { return Err(paimon::Error::Unsupported { message: format!( @@ -156,7 +149,6 @@ impl Catalog for TypedTestCatalog { } } -/// A stand-in engine: serves a fixed two-row in-memory table for `it`. #[derive(Debug)] struct FakeEngineResolver; @@ -188,14 +180,10 @@ impl TableEngineResolver for FakeEngineResolver { } struct TestEnv { - // Owns the warehouse directory for the duration of the test. _paimon_dir: TempDir, ctx: SQLContext, } -/// One SQLContext with catalog `cat`: a Paimon table `cat.shared_db.pt` -/// (two rows, two snapshots) plus a declared `iceberg-table` -/// `cat.shared_db.it` routed to a fake engine. async fn setup() -> TestEnv { let paimon_dir = TempDir::new().unwrap(); let warehouse = format!("file://{}", paimon_dir.path().display()); @@ -206,9 +194,7 @@ async fn setup() -> TestEnv { inner: fs_catalog, declared_types: HashMap::from([ ("it".to_string(), TableType::IcebergTable), - // Unknown to the engine: existence mirrors the resolver's miss. ("ghost".to_string(), TableType::IcebergTable), - // Write-statement target: raw get_table paths fail closed. ("ft".to_string(), TableType::IcebergTable), ]), }); @@ -222,7 +208,6 @@ async fn setup() -> TestEnv { )) .await .unwrap(); - // Two separate INSERTs -> two snapshots, so time travel has history. for stmt in [ format!("INSERT INTO {CATALOG}.{DB}.pt VALUES (1, 'a')"), format!("INSERT INTO {CATALOG}.{DB}.pt VALUES (2, 'b')"), @@ -275,7 +260,6 @@ async fn declared_engine_table_routes_to_engine() { .sql(&format!("SELECT id, payload FROM {CATALOG}.{DB}.it")) .await .unwrap(); - // Schema comes from the engine — `payload` only exists there. let names: Vec = df .schema() .fields() @@ -301,7 +285,6 @@ async fn cross_engine_join_plans_and_runs() { .collect() .await .unwrap(); - // pt has ids {1,2}; the engine table has {1,3}. assert_eq!(column_i32(&batches), vec![1]); } @@ -322,8 +305,6 @@ async fn missing_table_still_errors() { ); } -// Downcast-based paths — time travel, temp tables — must keep working -// with engines registered. #[tokio::test] async fn time_travel_still_works_with_engines_registered() { let env = setup().await; @@ -335,7 +316,6 @@ async fn time_travel_still_works_with_engines_registered() { .collect() .await .unwrap(); - // Snapshot 1 holds only the first INSERT. assert_eq!(column_i32(&batches), vec![1]); } @@ -387,7 +367,6 @@ async fn table_names_come_from_the_catalog_listing() { assert!(names.contains(&"it".to_string()), "{names:?}"); } -// Engine failures surface as the engine's error, never as "not found". #[derive(Debug)] struct BrokenResolver; @@ -421,9 +400,8 @@ async fn engine_errors_are_surfaced() { assert!(msg.contains("engine backend exploded"), "{msg}"); } -// A declared engine type with no registered engine takes the Paimon path. #[tokio::test] -async fn undeclared_engine_type_takes_paimon_path() { +async fn paimon_served_type_takes_paimon_path() { let paimon_dir = TempDir::new().unwrap(); let warehouse = format!("file://{}", paimon_dir.path().display()); let mut options = Options::new(); @@ -431,23 +409,20 @@ async fn undeclared_engine_type_takes_paimon_path() { let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); let typed_catalog = Arc::new(TypedTestCatalog { inner: fs_catalog, - declared_types: HashMap::from([("ot".to_string(), TableType::ObjectTable)]), + declared_types: HashMap::from([("mt".to_string(), TableType::MaterializedTable)]), }); let mut ctx = SQLContext::new(); ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); ctx.sql(&format!("CREATE SCHEMA {CATALOG}.{DB}")) .await .unwrap(); - // No engine registered at all: `ot` resolves through Paimon and fails as - // a plain missing table, exactly like before routing existed. - let Err(err) = ctx.sql(&format!("SELECT * FROM {CATALOG}.{DB}.ot")).await else { - panic!("undeclared engine type must fall through to Paimon"); + let Err(err) = ctx.sql(&format!("SELECT * FROM {CATALOG}.{DB}.mt")).await else { + panic!("a Paimon-served type must fall through to Paimon"); }; let msg = err.to_string().to_lowercase(); - assert!(msg.contains("ot") || msg.contains("not found"), "{msg}"); + assert!(msg.contains("mt") || msg.contains("not found"), "{msg}"); } -// Write statements against routed tables must fail closed. #[tokio::test] async fn writes_to_routed_tables_fail_closed() { let env = setup().await; @@ -463,7 +438,6 @@ async fn writes_to_routed_tables_fail_closed() { assert!(msg.contains(TableType::IcebergTable.as_str()), "{msg}"); } -// System tables are Paimon-only; routed tables get a clear error. #[tokio::test] async fn system_tables_on_routed_tables_error() { let env = setup().await; @@ -478,7 +452,6 @@ async fn system_tables_on_routed_tables_error() { assert!(msg.contains("cannot be read as a Paimon table"), "{msg}"); } -// Existence mirrors resolution: engine hit -> true, miss -> false. #[tokio::test] async fn table_exist_mirrors_the_resolver() { let env = setup().await; @@ -489,13 +462,12 @@ async fn table_exist_mirrors_the_resolver() { assert!(!schema.table_exist("it$snapshots")); } -// Only table types the Paimon reader cannot serve may be routed. #[tokio::test] async fn paimon_managed_types_cannot_be_routed() { let env = setup().await; let Err(err) = env.ctx.register_catalog_table_engine( CATALOG, - TableType::LanceTable, + TableType::FormatTable, Arc::new(FakeEngineResolver), ) else { panic!("registering an engine for a Paimon-managed type must fail"); @@ -504,8 +476,6 @@ async fn paimon_managed_types_cannot_be_routed() { assert!(msg.contains("served by the Paimon reader"), "{msg}"); } -// DML against a routed table is rejected even when the engine's provider -// is writable (the fake resolver returns a writable MemTable). #[tokio::test] async fn insert_into_routed_table_is_rejected() { let env = setup().await; @@ -524,3 +494,129 @@ async fn insert_into_routed_table_is_rejected() { "{err}" ); } + +#[tokio::test] +async fn object_and_lance_tables_route_to_engines() { + let paimon_dir = TempDir::new().unwrap(); + let warehouse = format!("file://{}", paimon_dir.path().display()); + let mut options = Options::new(); + options.set(CatalogOptions::WAREHOUSE, warehouse); + let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); + let typed_catalog = Arc::new(TypedTestCatalog { + inner: fs_catalog, + declared_types: HashMap::from([ + ("it".to_string(), TableType::ObjectTable), + ("lt".to_string(), TableType::LanceTable), + ]), + }); + let mut ctx = SQLContext::new(); + ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); + ctx.sql(&format!("CREATE SCHEMA {CATALOG}.{DB}")) + .await + .unwrap(); + for declared in [TableType::ObjectTable, TableType::LanceTable] { + ctx.register_catalog_table_engine(CATALOG, declared, Arc::new(FakeEngineResolver)) + .unwrap(); + } + + let batches = ctx + .sql(&format!("SELECT id FROM {CATALOG}.{DB}.it ORDER BY id")) + .await + .unwrap() + .collect() + .await + .unwrap(); + assert_eq!(column_i32(&batches), vec![1, 3]); +} + +#[tokio::test] +async fn time_travel_on_routed_tables_is_rejected() { + let env = setup().await; + env.ctx + .ctx() + .sql("SET datafusion.sql_parser.dialect = 'databricks'") + .await + .unwrap() + .collect() + .await + .unwrap(); + let Err(err) = env + .ctx + .ctx() + .sql(&format!( + "SELECT * FROM {CATALOG}.{DB}.it VERSION AS OF 999999" + )) + .await + else { + panic!("time travel on a routed table must not silently read current data"); + }; + let msg = err.to_string(); + assert!(msg.contains("time travel is not supported"), "{msg}"); + + let Err(err) = env + .ctx + .ctx() + .sql(&format!( + "SELECT * FROM {CATALOG}.{DB}.it TIMESTAMP AS OF '2020-01-01 00:00:00'" + )) + .await + else { + panic!("timestamp travel on a routed table must be rejected too"); + }; + assert!( + err.to_string().contains("time travel is not supported"), + "{err}" + ); +} + +#[tokio::test] +async fn session_time_travel_on_routed_tables_is_rejected() { + let env = setup().await; + env.ctx + .sql("SET 'paimon.scan.version' = '1'") + .await + .unwrap() + .collect() + .await + .unwrap(); + + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + else { + panic!("a session scan selector must not silently read current data"); + }; + assert!( + err.to_string().contains("time travel is not supported"), + "{err}" + ); + + env.ctx + .sql("RESET 'paimon.scan.version'") + .await + .unwrap() + .collect() + .await + .unwrap(); + env.ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + .expect("routing works again once the selector is reset"); + + env.ctx + .sql("SET 'paimon.incremental-between' = '1,5'") + .await + .unwrap() + .collect() + .await + .unwrap(); + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + else { + panic!("an unsupported scan option must not be silently ignored"); + }; + assert!(err.to_string().contains("incremental-between"), "{err}"); +} diff --git a/crates/paimon-rest-server/src/lib.rs b/crates/paimon-rest-server/src/lib.rs index 10d0efc28..403b42203 100644 --- a/crates/paimon-rest-server/src/lib.rs +++ b/crates/paimon-rest-server/src/lib.rs @@ -416,8 +416,8 @@ async fn create_table( async fn get_table(path: RestPath, Extension(state): Extension>) -> Response { let table = path.get("table"); let identifier = Identifier::new(path.get("db"), table.clone()); - // Raw metadata, not a constructed table: engine-served types (e.g. - // `iceberg-table`) must still be describable so clients can route them. + // Raw metadata, not a constructed table: engine-served types must stay + // describable so clients can route them. let (location, loaded_schema) = match state.catalog.fetch_table_schema(&identifier).await { Ok(loaded) => loaded, Err(e) => return error_response(e), diff --git a/crates/paimon-rest-server/tests/e2e.rs b/crates/paimon-rest-server/tests/e2e.rs index fc8454532..b25d32f96 100644 --- a/crates/paimon-rest-server/tests/e2e.rs +++ b/crates/paimon-rest-server/tests/e2e.rs @@ -430,8 +430,6 @@ async fn test_special_char_names() { assert!(cat.list_databases().await.unwrap().is_empty()); } -/// A table declared as an engine-served type stays describable over REST, so -/// the client can route it instead of reading it as Paimon. #[tokio::test] async fn declared_engine_type_routes_through_the_rest_catalog() { use paimon::catalog::RoutedTableLoad; @@ -465,7 +463,6 @@ async fn declared_engine_type_routes_through_the_rest_catalog() { "{routed:?}" ); - // Raw get_table still fails closed on both sides of the wire. let err = ctx .catalog .get_table(&identifier) @@ -476,3 +473,56 @@ async fn declared_engine_type_routes_through_the_rest_catalog() { "{err}" ); } + +#[tokio::test] +async fn creating_a_table_with_an_unknown_type_is_rejected() { + let ctx = setup().await; + ctx.catalog + .create_database("db", false, HashMap::new()) + .await + .expect("create database"); + + let mut payload = serde_json::to_value(append_only_schema()).expect("serialize schema"); + payload["options"]["type"] = serde_json::json!("iceberg-tabel"); + let schema: Schema = serde_json::from_value(payload).expect("deserialize schema"); + let identifier = Identifier::new("db", "typo_t"); + + ctx.catalog + .create_table(&identifier, schema, false) + .await + .expect_err("a misspelled type must not create a table"); + let tables = ctx.catalog.list_tables("db").await.expect("list tables"); + assert!(!tables.contains(&"typo_t".to_string()), "{tables:?}"); +} + +#[tokio::test] +async fn altering_the_declared_type_is_rejected() { + use paimon::spec::SchemaChange; + + let ctx = setup().await; + ctx.catalog + .create_database("db", false, HashMap::new()) + .await + .expect("create database"); + let identifier = Identifier::new("db", "t"); + ctx.catalog + .create_table(&identifier, append_only_schema(), false) + .await + .expect("create table"); + + ctx.catalog + .alter_table( + &identifier, + vec![SchemaChange::set_option( + "type".to_string(), + "iceberg-table".to_string(), + )], + false, + ) + .await + .expect_err("changing 'type' over REST must be rejected"); + ctx.catalog + .get_table(&identifier) + .await + .expect("still readable"); +} diff --git a/crates/paimon/src/catalog/filesystem.rs b/crates/paimon/src/catalog/filesystem.rs index dab90d544..07d36e969 100644 --- a/crates/paimon/src/catalog/filesystem.rs +++ b/crates/paimon/src/catalog/filesystem.rs @@ -26,7 +26,7 @@ use crate::common::{CatalogOptions, Options}; use crate::error::{ConfigInvalidSnafu, Error, Result}; use crate::io::cache::create_local_cache; use crate::io::FileIO; -use crate::spec::{CoreOptions, Schema, TableSchema, TableType}; +use crate::spec::{CoreOptions, Schema, TableSchema, TableType, TABLE_TYPE_OPTION}; use crate::table::{SchemaManager, Table}; use async_trait::async_trait; use bytes::Bytes; @@ -171,9 +171,8 @@ impl FileSystemCatalog { } /// Fetch the stored path and schema of an existing table, bypassing the - /// engine-type guard in [`Self::build_table`]. Routing inspects the - /// declared type through this, and a catalog server must keep serving raw - /// metadata for engine-served types so its clients can route them. + /// engine-type guard in [`Self::build_table`]: routing and catalog servers + /// need the declared type before deciding anything. pub async fn fetch_table_schema( &self, identifier: &Identifier, @@ -404,6 +403,8 @@ impl Catalog for FileSystemCatalog { ignore_if_exists: bool, ) -> Result<()> { identifier.validate()?; + // Never persist a type nothing can load. + CoreOptions::new(creation.options()).table_type()?; let table_path = self.table_path(identifier); @@ -511,6 +512,8 @@ impl Catalog for FileSystemCatalog { full_name: identifier.full_name(), })?; + reject_table_type_changes(current.options(), &changes)?; + let new_schema = current .apply_changes(changes) .map_err(|e| fill_table_name(e, identifier))?; @@ -518,6 +521,39 @@ impl Catalog for FileSystemCatalog { } } +/// The declared type picks the reader, so it is fixed at creation: flipping +/// it strands a populated table behind a reader that cannot see its data. +/// Only case-insensitive no-ops pass, matching Java `SchemaManager`. +fn reject_table_type_changes( + current_options: &HashMap, + changes: &[crate::spec::SchemaChange], +) -> Result<()> { + let current_type = current_options + .get(TABLE_TYPE_OPTION) + .map(String::as_str) + .unwrap_or_else(|| TableType::default().as_str()); + for change in changes { + match change { + crate::spec::SchemaChange::SetOption { key, value } + if key == TABLE_TYPE_OPTION && !value.eq_ignore_ascii_case(current_type) => + { + return Err(Error::Unsupported { + message: format!( + "changing '{TABLE_TYPE_OPTION}' from '{current_type}' to '{value}' is not supported" + ), + }); + } + crate::spec::SchemaChange::RemoveOption { key } if key == TABLE_TYPE_OPTION => { + return Err(Error::Unsupported { + message: format!("removing '{TABLE_TYPE_OPTION}' is not supported"), + }); + } + _ => {} + } + } + Ok(()) +} + /// `TableSchema::apply_changes` returns column errors without a table name; /// fill in the identifier's full name so the message identifies the table. fn fill_table_name(err: Error, identifier: &Identifier) -> Error { @@ -718,6 +754,115 @@ mod tests { assert!(escaped_path.exists()); } + #[tokio::test] + async fn test_alter_table_cannot_change_the_declared_type() { + use crate::spec::SchemaChange; + + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + let schema = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .build() + .unwrap(); + let identifier = Identifier::new("db1", "t"); + catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + + for change in [ + SchemaChange::set_option("type".to_string(), "iceberg-table".to_string()), + SchemaChange::set_option("type".to_string(), "delta-table".to_string()), + SchemaChange::remove_option("type".to_string()), + ] { + let err = catalog + .alter_table(&identifier, vec![change], false) + .await + .unwrap_err(); + assert!(matches!(err, Error::Unsupported { .. }), "{err:?}"); + } + assert!(catalog + .alter_table( + &identifier, + vec![SchemaChange::set_option( + "type".to_string(), + "TABLE".to_string(), + )], + false, + ) + .await + .is_ok()); + catalog.get_table(&identifier).await.unwrap(); + } + + #[tokio::test] + async fn test_create_table_rejects_an_unknown_type() { + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + let valid = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .build() + .unwrap(); + let mut payload = serde_json::to_value(&valid).unwrap(); + payload["options"] = serde_json::json!({ "type": "delta-table" }); + let schema: Schema = serde_json::from_value(payload).unwrap(); + let identifier = Identifier::new("db1", "delta_t"); + + let err = catalog + .create_table(&identifier, schema, false) + .await + .unwrap_err(); + assert!( + matches!(err, Error::Unsupported { ref message } if message.contains("delta-table")), + "{err:?}" + ); + assert!(!catalog.table_exists(&identifier).await.unwrap()); + } + + #[tokio::test] + async fn test_types_without_a_paimon_reader_fail_closed() { + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + + for (name, declared) in [("obj_t", "object-table"), ("lance_t", "lance-table")] { + let schema = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", declared) + .build() + .unwrap(); + let identifier = Identifier::new("db1", name); + catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + + let err = catalog.get_table(&identifier).await.unwrap_err(); + assert!( + matches!(err, Error::Unsupported { ref message } + if message.contains("cannot be read as a Paimon table")), + "{declared}: {err:?}" + ); + } + } + #[tokio::test] async fn test_declared_engine_type_routes_and_fails_closed() { use crate::catalog::RoutedTableLoad; @@ -742,7 +887,6 @@ mod tests { .await .unwrap(); - // Routed to the engine, exactly like the REST catalog does. let routed = catalog .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) .await @@ -752,7 +896,6 @@ mod tests { "{routed:?}" ); - // Raw get_table fails closed, so writes cannot reach a Paimon writer. let err = catalog.get_table(&identifier).await.unwrap_err(); assert!( matches!(err, Error::Unsupported { ref message } @@ -760,8 +903,6 @@ mod tests { "{err:?}" ); - // No engine registered for the type: still fails closed rather than - // silently reading it as Paimon. let err = catalog .load_table_routing(&identifier, &HashSet::new()) .await diff --git a/crates/paimon/src/spec/core_options.rs b/crates/paimon/src/spec/core_options.rs index 0acab7371..47fe33e65 100644 --- a/crates/paimon/src/spec/core_options.rs +++ b/crates/paimon/src/spec/core_options.rs @@ -925,6 +925,13 @@ impl<'a> CoreOptions<'a> { /// /// This is the semantic owner for selector mutual exclusion and strict /// numeric parsing. + /// Whether these options ask for a historical state of the table. A + /// malformed selector counts too, so callers that cannot honor time + /// travel reject rather than answer from the current state. + pub fn has_time_travel_selector(&self) -> bool { + !matches!(self.try_time_travel_selector(), Ok(None)) + } + pub(crate) fn try_time_travel_selector(&self) -> crate::Result>> { let selectors = self.configured_time_travel_selectors(); if selectors.len() > 1 { diff --git a/crates/paimon/src/spec/schema.rs b/crates/paimon/src/spec/schema.rs index 3238ccee0..8ebd5c28b 100644 --- a/crates/paimon/src/spec/schema.rs +++ b/crates/paimon/src/spec/schema.rs @@ -1155,6 +1155,9 @@ impl Schema { primary_keys: &[String], options: &HashMap, ) -> crate::Result<()> { + // Shared by create (`Schema::new`) and alter (`apply_changes`), so an + // unparsable type never reaches a metadata write. + CoreOptions::new(options).table_type()?; validate_no_reserved_field_names(fields)?; Self::validate_key_field_types(fields, primary_keys, options)?; Self::validate_row_tracking(primary_keys, options)?; diff --git a/crates/paimon/src/spec/table_type.rs b/crates/paimon/src/spec/table_type.rs index 40c147320..37e61f57c 100644 --- a/crates/paimon/src/spec/table_type.rs +++ b/crates/paimon/src/spec/table_type.rs @@ -53,11 +53,16 @@ impl TableType { } } - /// Whether the Paimon reader cannot serve this type, so it must be - /// routed to a table engine (see + /// Whether this type needs an engine of its own (see /// [`Catalog::load_table_routing`](crate::catalog::Catalog::load_table_routing)). + /// Java builds a dedicated table for each; this client has none, so + /// reading one as Paimon misreads it and writing could put Paimon + /// snapshots over foreign data. pub fn requires_table_engine(&self) -> bool { - matches!(self, TableType::IcebergTable) + matches!( + self, + TableType::ObjectTable | TableType::LanceTable | TableType::IcebergTable + ) } } @@ -119,14 +124,18 @@ mod tests { } #[test] - fn only_iceberg_requires_an_engine() { - assert!(TableType::IcebergTable.requires_table_engine()); + fn types_without_a_paimon_reader_require_an_engine() { + for table_type in [ + TableType::ObjectTable, + TableType::LanceTable, + TableType::IcebergTable, + ] { + assert!(table_type.requires_table_engine(), "{table_type}"); + } for table_type in [ TableType::Table, TableType::FormatTable, TableType::MaterializedTable, - TableType::ObjectTable, - TableType::LanceTable, ] { assert!(!table_type.requires_table_engine(), "{table_type}"); } diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 4fede10d6..93e9db410 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -1529,7 +1529,6 @@ async fn test_load_table_routing_returns_engine_for_declared_type() { "{routed:?}" ); - // No engine registered for the type: falls through to Paimon construction. let routed = ctx .catalog .load_table_routing(&identifier, &HashSet::new()) @@ -1554,7 +1553,6 @@ async fn test_load_table_routing_fails_closed_on_query_auth() { let identifier = Identifier::new("default", "authed_ice_t"); let engine_types = HashSet::from([TableType::IcebergTable]); - // Routing must fail closed on query-auth like every Paimon read. let err = ctx .catalog .load_table_routing(&identifier, &engine_types) @@ -1606,8 +1604,6 @@ async fn test_get_table_fails_closed_on_iceberg_table() { ctx.server .add_table_with_schema("default", "raw_ice_t", schema, "file:///unused"); - // Raw get_table paths fail closed on engine-served tables; reads go through - // load_table_routing + an engine. let err = ctx .catalog .get_table(&Identifier::new("default", "raw_ice_t")) @@ -1654,15 +1650,16 @@ async fn test_load_table_routing_rejects_unknown_declared_type() { use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; - let schema = Schema::builder() + let valid = Schema::builder() .column("id", DataType::BigInt(BigIntType::new())) - .option("type", "delta-table") .build() .unwrap(); + let mut payload = serde_json::to_value(&valid).unwrap(); + payload["options"] = serde_json::json!({ "type": "delta-table" }); + let schema: Schema = serde_json::from_value(payload).unwrap(); ctx.server .add_table_with_schema("default", "delta_t", schema, "file:///unused"); - // A type this client does not know is not silently read as Paimon. let err = ctx .catalog .load_table_routing(&Identifier::new("default", "delta_t"), &HashSet::new()) From 70f02c764a304dba2c4938f04ca87206a6e764ff Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 21 Aug 2026 03:30:11 -0400 Subject: [PATCH 5/6] fix: reject stored scan selectors and every version clause on routed tables --- crates/integrations/datafusion/src/catalog.rs | 99 +++++++++--- crates/integrations/datafusion/src/lib.rs | 5 +- .../datafusion/src/relation_planner.rs | 30 ++-- .../datafusion/src/sql_context.rs | 11 +- .../datafusion/tests/table_type_routing.rs | 151 +++++++++++++++++- crates/paimon-rest-server/tests/e2e.rs | 2 +- crates/paimon/src/catalog/filesystem.rs | 98 +++++++++++- crates/paimon/src/catalog/mod.rs | 31 +++- .../paimon/src/catalog/rest/rest_catalog.rs | 7 +- crates/paimon/src/spec/core_options.rs | 20 +++ crates/paimon/tests/rest_catalog_test.rs | 4 +- 11 files changed, 401 insertions(+), 57 deletions(-) diff --git a/crates/integrations/datafusion/src/catalog.rs b/crates/integrations/datafusion/src/catalog.rs index defdc070c..6bc40ac1b 100644 --- a/crates/integrations/datafusion/src/catalog.rs +++ b/crates/integrations/datafusion/src/catalog.rs @@ -29,6 +29,7 @@ use datafusion::datasource::{TableProvider, TableType}; use datafusion::error::Result as DFResult; use datafusion::execution::SessionState; use datafusion::logical_expr::{expr_fn::cast, Expr, LogicalPlan, LogicalPlanBuilder}; +use datafusion::prelude::SessionContext; use datafusion::sql::planner::IdentNormalizer; use datafusion::sql::sqlparser::ast::{Ident, ObjectName, Query, Statement, Visit, Visitor}; use datafusion::sql::sqlparser::dialect::GenericDialect; @@ -48,17 +49,41 @@ pub(crate) type SessionStateProvider = Arc Option + Se /// providers, so registrations stay visible to schemas obtained earlier. type TableEngines = Arc>>>; +/// What an engine is asked to resolve. Non-exhaustive so later releases can +/// carry more of the request — a snapshot selector, say — without breaking +/// existing resolvers. +#[derive(Debug, Clone)] +#[non_exhaustive] +pub struct EngineTableRequest { + /// Database holding the table. + pub database: String, + /// Table name. + pub table: String, + /// The type the table's metadata declares. + pub declared: PaimonTableType, +} + +impl EngineTableRequest { + /// A request for `database.table`, declared as `declared`. + pub fn new(database: String, table: String, declared: PaimonTableType) -> Self { + Self { + database, + table, + declared, + } + } +} + /// Resolves tables owned by another engine (see /// [`PaimonCatalogProvider::register_table_engine`]). `Ok(None)` means not /// found; errors propagate, so an engine failure never looks like a missing /// table. #[async_trait] pub trait TableEngineResolver: Debug + Send + Sync { - /// Resolve `database.table` to the engine's table provider. + /// Resolve a request to the engine's table provider. async fn resolve_table( &self, - database: &str, - table: &str, + request: &EngineTableRequest, ) -> DFResult>>; } @@ -132,11 +157,47 @@ impl TableProvider for ReadOnlyTableProvider { } } +/// Register `resolver` as the engine for `table_type` on the Paimon catalog +/// named `catalog_name`. +/// +/// Also installs [`PaimonRelationPlanner`](crate::PaimonRelationPlanner) on +/// `ctx`. Without it DataFusion drops `VERSION`/`TIMESTAMP AS OF` before this +/// crate sees the query, and a historical read of a routed table would return +/// current data; installing it here makes that impossible to forget. +/// [`SQLContext`](crate::SQLContext) already installs the planner, and its own +/// `register_catalog_table_engine` is equivalent to this. +pub fn register_catalog_table_engine( + ctx: &SessionContext, + catalog_name: &str, + table_type: PaimonTableType, + resolver: Arc, +) -> DFResult<()> { + ctx.register_relation_planner(Arc::new(crate::PaimonRelationPlanner::new()))?; + let provider = ctx + .catalog(catalog_name) + .ok_or_else(|| plan_datafusion_err!("Unknown catalog '{catalog_name}'"))?; + provider + .downcast_ref::() + .ok_or_else(|| plan_datafusion_err!("Catalog '{catalog_name}' is not a Paimon catalog"))? + .register_table_engine(table_type, resolver) +} + /// Provides an interface to manage and access multiple schemas (databases) /// within a Paimon [`Catalog`]. /// /// This provider uses lazy loading - databases and tables are fetched /// on-demand from the catalog, ensuring data is always fresh. +/// +/// # Table-version clauses +/// +/// SQL queries using `VERSION`/`TIMESTAMP AS OF` need +/// [`PaimonRelationPlanner`](crate::PaimonRelationPlanner) installed on the +/// session; DataFusion's default planner drops the clause before this crate +/// sees it, so the query would read current data. [`SQLContext`](crate::SQLContext) +/// installs it, as does +/// [`register_catalog_table_engine`]. A plain `SessionContext` querying Paimon +/// tables directly must install it with +/// `ctx.register_relation_planner(Arc::new(PaimonRelationPlanner::new()))`. pub struct PaimonCatalogProvider { catalog_name: Option, /// Reference to the Paimon catalog. @@ -201,7 +262,7 @@ impl PaimonCatalogProvider { /// Paimon path unchanged. Kept inside the provider so the registered /// catalog type never changes and downcast-based paths (temp tables, /// time travel) keep working. - pub fn register_table_engine( + pub(crate) fn register_table_engine( &self, table_type: PaimonTableType, resolver: Arc, @@ -611,7 +672,8 @@ impl SchemaProvider for PaimonSchemaProvider { await_with_runtime(async move { let engine_types: HashSet = table_engines.keys().copied().collect(); match catalog.load_table_routing(&identifier, &engine_types).await { - Ok(paimon::catalog::RoutedTableLoad::Engine(declared)) => { + Ok(paimon::catalog::RoutedTableLoad::Engine(engine)) => { + let declared = engine.declared(); if branch.is_some() { return Err(plan_datafusion_err!( "branches are not supported for '{}' tables ('{}')", @@ -625,22 +687,18 @@ impl SchemaProvider for PaimonSchemaProvider { .read() .unwrap_or_else(|e| e.into_inner()) .clone(); - let session_options = paimon::spec::CoreOptions::new(&session_options); - session_options - .validate_scan_options() + paimon::spec::CoreOptions::new(&session_options) + .ensure_engine_can_serve(&identifier.full_name()) .map_err(to_datafusion_error)?; - if session_options.has_time_travel_selector() { - return Err(plan_datafusion_err!( - "time travel is not supported for routed '{}' 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()) + .resolve_table(&EngineTableRequest::new( + identifier.database().to_string(), + identifier.object().to_string(), + declared, + )) .await?; // Read-only wrap: DML must not reach the engine provider. Ok(resolved.map(|inner| { @@ -802,14 +860,19 @@ impl SchemaProvider for PaimonSchemaProvider { async move { let engine_types: HashSet = engines.keys().copied().collect(); match catalog.load_table_routing(&identifier, &engine_types).await { - Ok(paimon::catalog::RoutedTableLoad::Engine(declared)) => { + Ok(paimon::catalog::RoutedTableLoad::Engine(engine)) => { + let declared = engine.declared(); // Paimon-only; `table()` rejects them here too. if branch.is_some() || has_system_suffix { return false; } match engines.get(&declared) { Some(resolver) => match resolver - .resolve_table(identifier.database(), identifier.object()) + .resolve_table(&EngineTableRequest::new( + identifier.database().to_string(), + identifier.object().to_string(), + declared, + )) .await { Ok(table) => table.is_some(), diff --git a/crates/integrations/datafusion/src/lib.rs b/crates/integrations/datafusion/src/lib.rs index 784215e0b..985ad31fb 100644 --- a/crates/integrations/datafusion/src/lib.rs +++ b/crates/integrations/datafusion/src/lib.rs @@ -74,7 +74,10 @@ pub(crate) type DynamicOptions = Arc>>; pub use blob_reader::BlobReaderRegistry; pub use blob_view::register_blob_view; -pub use catalog::{PaimonCatalogProvider, PaimonSchemaProvider, TableEngineResolver}; +pub use catalog::{ + register_catalog_table_engine, EngineTableRequest, PaimonCatalogProvider, PaimonSchemaProvider, + TableEngineResolver, +}; pub use error::to_datafusion_error; #[cfg(feature = "fulltext")] pub use full_text_search::{register_full_text_search, FullTextSearchFunction}; diff --git a/crates/integrations/datafusion/src/relation_planner.rs b/crates/integrations/datafusion/src/relation_planner.rs index 2dae8d0fb..60978e3dd 100644 --- a/crates/integrations/datafusion/src/relation_planner.rs +++ b/crates/integrations/datafusion/src/relation_planner.rs @@ -72,10 +72,8 @@ impl RelationPlanner for PaimonRelationPlanner { return Ok(RelationPlanning::Original(Box::new(relation))); }; - let extra_options = match version { - Some(TableVersion::VersionAsOf(expr)) => resolve_version_as_of(expr)?, - Some(TableVersion::TimestampAsOf(expr)) => resolve_timestamp_as_of(expr)?, - _ => return Ok(RelationPlanning::Original(Box::new(relation))), + let Some(version) = version else { + return Ok(RelationPlanning::Original(Box::new(relation))); }; // Resolve the table reference. @@ -85,16 +83,24 @@ impl RelationPlanner for PaimonRelationPlanner { .get_table_source(table_ref.clone())?; let provider = source_as_provider(&source)?; + // Every version clause, not just the two resolved below: falling + // through drops the clause silently. + if let Some(routed) = provider.downcast_ref::() { + return Err(plan_datafusion_err!( + "time travel is not supported for routed '{}' tables ('{}')", + routed.declared, + routed.table_name + )); + } + + let extra_options = match version { + TableVersion::VersionAsOf(expr) => resolve_version_as_of(expr)?, + TableVersion::TimestampAsOf(expr) => resolve_timestamp_as_of(expr)?, + _ => return Ok(RelationPlanning::Original(Box::new(relation))), + }; + // Check if this is a Paimon table. let Some(paimon_provider) = provider.downcast_ref::() else { - if let Some(routed) = provider.downcast_ref::() { - // Falling through drops the version clause silently. - return Err(plan_datafusion_err!( - "time travel is not supported for routed '{}' tables ('{}')", - routed.declared, - routed.table_name - )); - } return Ok(RelationPlanning::Original(Box::new(relation))); }; diff --git a/crates/integrations/datafusion/src/sql_context.rs b/crates/integrations/datafusion/src/sql_context.rs index bb30c7348..f441c1577 100644 --- a/crates/integrations/datafusion/src/sql_context.rs +++ b/crates/integrations/datafusion/src/sql_context.rs @@ -363,16 +363,7 @@ impl SQLContext { table_type: paimon::spec::TableType, resolver: Arc, ) -> DFResult<()> { - let catalog_provider = self - .ctx - .catalog(catalog_name) - .ok_or_else(|| DataFusionError::Plan(format!("Unknown catalog '{catalog_name}'")))?; - let paimon_provider = catalog_provider - .downcast_ref::() - .ok_or_else(|| { - DataFusionError::Plan(format!("Catalog '{catalog_name}' is not a Paimon catalog")) - })?; - paimon_provider.register_table_engine(table_type, resolver) + crate::catalog::register_catalog_table_engine(&self.ctx, catalog_name, table_type, resolver) } /// Deregisters a temporary table or view. diff --git a/crates/integrations/datafusion/tests/table_type_routing.rs b/crates/integrations/datafusion/tests/table_type_routing.rs index 34a31df80..557a9d9d4 100644 --- a/crates/integrations/datafusion/tests/table_type_routing.rs +++ b/crates/integrations/datafusion/tests/table_type_routing.rs @@ -28,7 +28,7 @@ use paimon::catalog::{Catalog, Database, Identifier, RoutedTableLoad}; use paimon::spec::{Schema as PaimonSchema, SchemaChange, TableType}; use paimon::table::Table; use paimon::{CatalogOptions, FileSystemCatalog, Options, Result as PaimonResult}; -use paimon_datafusion::{SQLContext, TableEngineResolver}; +use paimon_datafusion::{EngineTableRequest, SQLContext, TableEngineResolver}; use tempfile::TempDir; const CATALOG: &str = "cat"; @@ -91,7 +91,12 @@ impl Catalog for TypedTestCatalog { ) -> PaimonResult { if let Some(declared) = self.declared_types.get(identifier.object()) { if engine_types.contains(declared) { - return Ok(RoutedTableLoad::Engine(*declared)); + let options = HashMap::new(); + return RoutedTableLoad::engine( + *declared, + &paimon::spec::CoreOptions::new(&options), + &identifier.full_name(), + ); } } Ok(RoutedTableLoad::Paimon(Box::new( @@ -156,10 +161,9 @@ struct FakeEngineResolver; impl TableEngineResolver for FakeEngineResolver { async fn resolve_table( &self, - _database: &str, - table: &str, + request: &EngineTableRequest, ) -> DFResult>> { - if table != "it" { + if request.table != "it" { return Ok(None); } let schema = Arc::new(ArrowSchema::new(vec![ @@ -374,8 +378,7 @@ struct BrokenResolver; impl TableEngineResolver for BrokenResolver { async fn resolve_table( &self, - _database: &str, - _table: &str, + _request: &EngineTableRequest, ) -> DFResult>> { Err(DataFusionError::Execution( "engine backend exploded".to_string(), @@ -620,3 +623,137 @@ async fn session_time_travel_on_routed_tables_is_rejected() { }; assert!(err.to_string().contains("incremental-between"), "{err}"); } + +#[tokio::test] +async fn every_version_clause_on_routed_tables_is_rejected() { + let env = setup().await; + env.ctx + .ctx() + .sql("SET datafusion.sql_parser.dialect = 'databricks'") + .await + .unwrap() + .collect() + .await + .unwrap(); + + for clause in [ + "VERSION AS OF 999999", + "TIMESTAMP AS OF '2020-01-01 00:00:00'", + "FOR SYSTEM_TIME AS OF '2020-01-01 00:00:00'", + ] { + let Err(err) = env + .ctx + .ctx() + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it {clause}")) + .await + else { + panic!("{clause} must not silently read current data"); + }; + assert!( + err.to_string().contains("time travel is not supported"), + "{clause}: {err}" + ); + } +} + +#[tokio::test] +async fn show_create_on_routed_tables_reports_the_declared_type() { + let env = setup().await; + let Err(err) = env + .ctx + .sql(&format!("SHOW CREATE TABLE {CATALOG}.{DB}.it")) + .await + else { + panic!("Paimon DDL would misrepresent an engine-served table"); + }; + let msg = err.to_string(); + assert!(msg.contains("iceberg-table"), "{msg}"); + assert!(msg.contains("cannot be read as a Paimon table"), "{msg}"); +} + +#[tokio::test] +async fn session_query_auth_blocks_routed_reads() { + let env = setup().await; + env.ctx + .sql("SET 'paimon.query-auth.enabled' = 'true'") + .await + .unwrap() + .collect() + .await + .unwrap(); + + let Err(err) = env + .ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + else { + panic!("query-auth must block a routed read, not just a Paimon one"); + }; + assert!(err.to_string().contains("query-auth"), "{err}"); + + env.ctx + .sql("RESET 'paimon.query-auth.enabled'") + .await + .unwrap() + .collect() + .await + .unwrap(); + env.ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")) + .await + .expect("routing works again once the flag is reset"); +} + +#[tokio::test] +async fn registering_on_a_raw_session_installs_the_planner() { + use datafusion::prelude::SessionContext; + use paimon_datafusion::{register_catalog_table_engine, PaimonCatalogProvider}; + + let paimon_dir = TempDir::new().unwrap(); + let warehouse = format!("file://{}", paimon_dir.path().display()); + let mut options = Options::new(); + options.set(CatalogOptions::WAREHOUSE, warehouse); + let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); + fs_catalog + .create_database(DB, false, HashMap::new()) + .await + .unwrap(); + let typed_catalog = Arc::new(TypedTestCatalog { + inner: fs_catalog, + declared_types: HashMap::from([("it".to_string(), TableType::IcebergTable)]), + }); + + // No SQLContext: the raw path a caller might take. + let ctx = SessionContext::new(); + ctx.register_catalog( + CATALOG, + Arc::new(PaimonCatalogProvider::new( + Some(CATALOG.to_string()), + typed_catalog, + Default::default(), + Default::default(), + None, + )), + ); + register_catalog_table_engine( + &ctx, + CATALOG, + TableType::IcebergTable, + Arc::new(FakeEngineResolver), + ) + .unwrap(); + ctx.sql("SET datafusion.sql_parser.dialect = 'databricks'") + .await + .unwrap() + .collect() + .await + .unwrap(); + + let Err(err) = ctx + .sql(&format!("SELECT * FROM {CATALOG}.{DB}.it VERSION AS OF 1")) + .await + else { + panic!("registration must install the planner, so this cannot read current data"); + }; + assert!(err.to_string().contains("not supported"), "{err}"); +} diff --git a/crates/paimon-rest-server/tests/e2e.rs b/crates/paimon-rest-server/tests/e2e.rs index b25d32f96..94f8fa277 100644 --- a/crates/paimon-rest-server/tests/e2e.rs +++ b/crates/paimon-rest-server/tests/e2e.rs @@ -459,7 +459,7 @@ async fn declared_engine_type_routes_through_the_rest_catalog() { .await .expect("routing must reach the declared type"); assert!( - matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); diff --git a/crates/paimon/src/catalog/filesystem.rs b/crates/paimon/src/catalog/filesystem.rs index 07d36e969..d759cd576 100644 --- a/crates/paimon/src/catalog/filesystem.rs +++ b/crates/paimon/src/catalog/filesystem.rs @@ -368,8 +368,11 @@ impl Catalog for FileSystemCatalog { if engine_types.contains(&declared) { // Neither this client nor an engine can enforce the // server-side row filter / column mask. - options.ensure_read_authorized()?; - return Ok(crate::catalog::RoutedTableLoad::Engine(declared)); + return crate::catalog::RoutedTableLoad::engine( + declared, + &options, + &identifier.full_name(), + ); } self.build_table(identifier, table_path, schema) .map(|table| crate::catalog::RoutedTableLoad::Paimon(Box::new(table))) @@ -831,6 +834,95 @@ mod tests { assert!(!catalog.table_exists(&identifier).await.unwrap()); } + #[tokio::test] + async fn test_stored_scan_selectors_block_routing() { + use crate::catalog::RoutedTableLoad; + use std::collections::HashSet; + + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + + for (name, key, value) in [ + ("ice_v", "scan.version", "1"), + ("ice_s", "scan.snapshot-id", "1"), + ("ice_i", "incremental-between", "1,5"), + ] { + let schema = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", "iceberg-table") + .option(key, value) + .build() + .unwrap(); + let identifier = Identifier::new("db1", name); + catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + + let err = catalog + .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) + .await + .unwrap_err(); + assert!(matches!(err, Error::Unsupported { .. }), "{key}: {err:?}"); + } + + let schema = Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", "iceberg-table") + .build() + .unwrap(); + let identifier = Identifier::new("db1", "ice_plain"); + catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + let routed = catalog + .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) + .await + .unwrap(); + assert!(matches!(routed, RoutedTableLoad::Engine(_)), "{routed:?}"); + } + + #[tokio::test] + async fn test_routing_cannot_skip_the_read_guards() { + use crate::catalog::RoutedTableLoad; + use crate::spec::CoreOptions; + + // The only constructor of the `Engine` variant, so a third-party + // catalog cannot route past these guards either. + for (key, value) in [ + ("query-auth.enabled", "true"), + ("scan.version", "1"), + ("incremental-between", "1,5"), + ] { + let options = HashMap::from([(key.to_string(), value.to_string())]); + let err = RoutedTableLoad::engine( + TableType::IcebergTable, + &CoreOptions::new(&options), + "db1.t", + ) + .unwrap_err(); + assert!(matches!(err, Error::Unsupported { .. }), "{key}: {err:?}"); + } + + let options = HashMap::new(); + assert!(RoutedTableLoad::engine( + TableType::IcebergTable, + &CoreOptions::new(&options), + "db1.t", + ) + .is_ok()); + } + #[tokio::test] async fn test_types_without_a_paimon_reader_fail_closed() { let (_temp_dir, catalog) = create_test_catalog(); @@ -892,7 +984,7 @@ mod tests { .await .unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); diff --git a/crates/paimon/src/catalog/mod.rs b/crates/paimon/src/catalog/mod.rs index e20b1ff38..a0472cdcf 100644 --- a/crates/paimon/src/catalog/mod.rs +++ b/crates/paimon/src/catalog/mod.rs @@ -271,7 +271,36 @@ pub enum RoutedTableLoad { /// A constructed Paimon table (boxed: far larger than the other variant). Paimon(Box
), /// The declared type is in `engine_types`; construction was skipped. - Engine(TableType), + Engine(EngineTable), +} + +/// A table handed to a registered engine. Only [`RoutedTableLoad::engine`] +/// can build one, so no catalog — including a third-party implementation — +/// routes a table without the checks that constructor performs. +#[derive(Debug)] +pub struct EngineTable { + declared: TableType, +} + +impl EngineTable { + /// The type the table's metadata declares. + pub fn declared(&self) -> TableType { + self.declared + } +} + +impl RoutedTableLoad { + /// Route `full_name` to the engine registered for `declared`, refusing + /// what [`CoreOptions::ensure_engine_can_serve`](crate::spec::CoreOptions::ensure_engine_can_serve) + /// refuses. + pub fn engine( + declared: TableType, + options: &crate::spec::CoreOptions<'_>, + full_name: &str, + ) -> Result { + options.ensure_engine_can_serve(full_name)?; + Ok(Self::Engine(EngineTable { declared })) + } } /// Catalog API for reading and writing metadata (databases, tables) in Paimon. diff --git a/crates/paimon/src/catalog/rest/rest_catalog.rs b/crates/paimon/src/catalog/rest/rest_catalog.rs index a15a53777..352220705 100644 --- a/crates/paimon/src/catalog/rest/rest_catalog.rs +++ b/crates/paimon/src/catalog/rest/rest_catalog.rs @@ -227,8 +227,11 @@ impl Catalog for RESTCatalog { if engine_types.contains(&declared) { // Neither this client nor an engine can enforce the // server-side row filter / column mask. - options.ensure_read_authorized()?; - return Ok(crate::catalog::RoutedTableLoad::Engine(declared)); + return crate::catalog::RoutedTableLoad::engine( + declared, + &options, + &identifier.full_name(), + ); } } RESTEnv::build_table( diff --git a/crates/paimon/src/spec/core_options.rs b/crates/paimon/src/spec/core_options.rs index 47fe33e65..cc57ccef8 100644 --- a/crates/paimon/src/spec/core_options.rs +++ b/crates/paimon/src/spec/core_options.rs @@ -925,6 +925,26 @@ impl<'a> CoreOptions<'a> { /// /// This is the semantic owner for selector mutual exclusion and strict /// numeric parsing. + /// Fails when these options forbid the read outright, or ask for + /// something a table engine cannot honor: a scan option this client does + /// not support, or a historical state. Dropping any of them would answer + /// with unfiltered or current data instead. + /// + /// Both the table's stored options and a session's options go through + /// here, so neither source can skip a check the other applies. + pub fn ensure_engine_can_serve(&self, full_name: &str) -> crate::Result<()> { + self.ensure_read_authorized()?; + self.validate_scan_options()?; + if self.has_time_travel_selector() { + return Err(crate::Error::Unsupported { + message: format!( + "time travel is not supported for engine-served table '{full_name}'" + ), + }); + } + Ok(()) + } + /// Whether these options ask for a historical state of the table. A /// malformed selector counts too, so callers that cannot honor time /// travel reject rather than answer from the current state. diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 93e9db410..d94d0f6bd 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -1525,7 +1525,7 @@ async fn test_load_table_routing_returns_engine_for_declared_type() { .await .unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); @@ -1640,7 +1640,7 @@ async fn test_load_table_routing_parses_declared_type_case_insensitively() { .await .unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(TableType::IcebergTable)), + matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); } From 76fbd375a15122c9bd865bfaae5aae342d166d24 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 21 Aug 2026 10:39:54 -0400 Subject: [PATCH 6/6] refactor: classify tables in core, keep engine registration in DataFusion --- crates/integrations/datafusion/src/catalog.rs | 48 ++++++-------- .../datafusion/src/sql_context.rs | 2 +- .../datafusion/tests/table_type_routing.rs | 41 +++++++++--- crates/paimon-rest-server/tests/e2e.rs | 7 +- crates/paimon/src/catalog/filesystem.rs | 52 ++++----------- crates/paimon/src/catalog/mod.rs | 58 +++++++++-------- .../paimon/src/catalog/rest/rest_catalog.rs | 14 ++-- crates/paimon/src/spec/schema.rs | 3 +- crates/paimon/src/spec/table_type.rs | 8 +-- crates/paimon/tests/rest_catalog_test.rs | 65 +++++-------------- 10 files changed, 127 insertions(+), 171 deletions(-) diff --git a/crates/integrations/datafusion/src/catalog.rs b/crates/integrations/datafusion/src/catalog.rs index 6bc40ac1b..cbee1a6e7 100644 --- a/crates/integrations/datafusion/src/catalog.rs +++ b/crates/integrations/datafusion/src/catalog.rs @@ -55,16 +55,13 @@ type TableEngines = Arc Self { Self { database, @@ -160,12 +157,10 @@ impl TableProvider for ReadOnlyTableProvider { /// Register `resolver` as the engine for `table_type` on the Paimon catalog /// named `catalog_name`. /// -/// Also installs [`PaimonRelationPlanner`](crate::PaimonRelationPlanner) on -/// `ctx`. Without it DataFusion drops `VERSION`/`TIMESTAMP AS OF` before this -/// crate sees the query, and a historical read of a routed table would return -/// current data; installing it here makes that impossible to forget. -/// [`SQLContext`](crate::SQLContext) already installs the planner, and its own -/// `register_catalog_table_engine` is equivalent to this. +/// Also installs [`PaimonRelationPlanner`](crate::PaimonRelationPlanner), so +/// version clauses cannot slip past this crate (see [`PaimonCatalogProvider`]). +/// [`SQLContext::register_catalog_table_engine`](crate::SQLContext::register_catalog_table_engine) +/// is equivalent. pub fn register_catalog_table_engine( ctx: &SessionContext, catalog_name: &str, @@ -286,7 +281,6 @@ impl PaimonCatalogProvider { Arc::clone(&self.table_engines) } - /// The Paimon-side schema resolution. fn paimon_schema(&self, name: &str) -> Option> { let catalog = Arc::clone(&self.catalog); let table_engines = self.table_engines(); @@ -670,10 +664,9 @@ impl SchemaProvider for PaimonSchemaProvider { .unwrap_or_else(|e| e.into_inner()) .clone(); await_with_runtime(async move { - let engine_types: HashSet = table_engines.keys().copied().collect(); - match catalog.load_table_routing(&identifier, &engine_types).await { - Ok(paimon::catalog::RoutedTableLoad::Engine(engine)) => { - let declared = engine.declared(); + match catalog.load_table(&identifier).await { + Ok(paimon::catalog::LoadedTable::External(external)) => { + let declared = external.declared(); if branch.is_some() { return Err(plan_datafusion_err!( "branches are not supported for '{}' tables ('{}')", @@ -690,9 +683,13 @@ impl SchemaProvider for PaimonSchemaProvider { paimon::spec::CoreOptions::new(&session_options) .ensure_engine_can_serve(&identifier.full_name()) .map_err(to_datafusion_error)?; - let resolver = table_engines - .get(&declared) - .expect("declared type came from this engine map"); + let resolver = table_engines.get(&declared).ok_or_else(|| { + plan_datafusion_err!( + "no table engine is registered for '{}' tables ('{}')", + declared, + identifier.full_name() + ) + })?; let resolved = resolver .resolve_table(&EngineTableRequest::new( identifier.database().to_string(), @@ -700,7 +697,6 @@ impl SchemaProvider for PaimonSchemaProvider { declared, )) .await?; - // Read-only wrap: DML must not reach the engine provider. Ok(resolved.map(|inner| { Arc::new(ReadOnlyTableProvider { inner, @@ -709,7 +705,7 @@ impl SchemaProvider for PaimonSchemaProvider { }) as Arc })) } - Ok(paimon::catalog::RoutedTableLoad::Paimon(table)) => { + Ok(paimon::catalog::LoadedTable::Paimon(table)) => { let mut table = *table; if let Some(branch) = branch.as_deref() { table = table @@ -767,7 +763,7 @@ impl SchemaProvider for PaimonSchemaProvider { identifier.full_name() ) })?; - validate_view_dependencies(&catalog, &catalog_name, &view, &engine_types) + validate_view_dependencies(&catalog, &catalog_name, &view) .await?; let mut state = session_state .and_then(|provider| provider()) @@ -858,10 +854,9 @@ impl SchemaProvider for PaimonSchemaProvider { .clone(); block_on_with_runtime( async move { - let engine_types: HashSet = engines.keys().copied().collect(); - match catalog.load_table_routing(&identifier, &engine_types).await { - Ok(paimon::catalog::RoutedTableLoad::Engine(engine)) => { - let declared = engine.declared(); + match catalog.load_table(&identifier).await { + Ok(paimon::catalog::LoadedTable::External(external)) => { + let declared = external.declared(); // Paimon-only; `table()` rejects them here too. if branch.is_some() || has_system_suffix { return false; @@ -889,7 +884,7 @@ impl SchemaProvider for PaimonSchemaProvider { None => false, } } - Ok(paimon::catalog::RoutedTableLoad::Paimon(table)) => { + Ok(paimon::catalog::LoadedTable::Paimon(table)) => { if let Some(branch) = branch.as_deref() { if is_branches_table { return true; @@ -992,7 +987,6 @@ async fn validate_view_dependencies( catalog: &Arc, catalog_name: &str, root: &View, - engine_types: &HashSet, ) -> DFResult<()> { let mut queue = VecDeque::from([root.clone()]); let mut loaded = HashSet::from([root.identifier().clone()]); @@ -1003,7 +997,7 @@ async fn validate_view_dependencies( let mut view_dependencies = Vec::new(); for identifier in candidates { // Routed engine tables count as existing dependencies. - match catalog.load_table_routing(&identifier, engine_types).await { + match catalog.load_table(&identifier).await { Ok(_) => continue, Err(paimon::Error::TableNotExist { .. }) | Err(paimon::Error::Unsupported { .. }) => {} diff --git a/crates/integrations/datafusion/src/sql_context.rs b/crates/integrations/datafusion/src/sql_context.rs index f441c1577..cf306f2c7 100644 --- a/crates/integrations/datafusion/src/sql_context.rs +++ b/crates/integrations/datafusion/src/sql_context.rs @@ -348,7 +348,7 @@ impl SQLContext { paimon_provider.register_temp_table(&database, &table_name, table) } - /// Whether `name` was registered through the Paimon registration path. + /// Whether `name` is a Paimon catalog, rather than one DataFusion holds. pub fn is_paimon_catalog(&self, name: &str) -> bool { self.catalogs.contains_key(name) } diff --git a/crates/integrations/datafusion/tests/table_type_routing.rs b/crates/integrations/datafusion/tests/table_type_routing.rs index 557a9d9d4..f75ae8f3d 100644 --- a/crates/integrations/datafusion/tests/table_type_routing.rs +++ b/crates/integrations/datafusion/tests/table_type_routing.rs @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use async_trait::async_trait; @@ -24,7 +24,7 @@ use datafusion::arrow::datatypes::{DataType, Field, Schema as ArrowSchema}; use datafusion::arrow::record_batch::RecordBatch; use datafusion::datasource::{MemTable, TableProvider}; use datafusion::error::{DataFusionError, Result as DFResult}; -use paimon::catalog::{Catalog, Database, Identifier, RoutedTableLoad}; +use paimon::catalog::{Catalog, Database, Identifier, LoadedTable}; use paimon::spec::{Schema as PaimonSchema, SchemaChange, TableType}; use paimon::table::Table; use paimon::{CatalogOptions, FileSystemCatalog, Options, Result as PaimonResult}; @@ -84,22 +84,18 @@ impl Catalog for TypedTestCatalog { self.inner.get_table(identifier).await } - async fn load_table_routing( - &self, - identifier: &Identifier, - engine_types: &HashSet, - ) -> PaimonResult { + async fn load_table(&self, identifier: &Identifier) -> PaimonResult { if let Some(declared) = self.declared_types.get(identifier.object()) { - if engine_types.contains(declared) { + if declared.requires_table_engine() { let options = HashMap::new(); - return RoutedTableLoad::engine( + return LoadedTable::external( *declared, &paimon::spec::CoreOptions::new(&options), &identifier.full_name(), ); } } - Ok(RoutedTableLoad::Paimon(Box::new( + Ok(LoadedTable::Paimon(Box::new( self.get_table(identifier).await?, ))) } @@ -757,3 +753,28 @@ async fn registering_on_a_raw_session_installs_the_planner() { }; assert!(err.to_string().contains("not supported"), "{err}"); } + +#[tokio::test] +async fn an_external_type_without_an_engine_says_so() { + let paimon_dir = TempDir::new().unwrap(); + let warehouse = format!("file://{}", paimon_dir.path().display()); + let mut options = Options::new(); + options.set(CatalogOptions::WAREHOUSE, warehouse); + let fs_catalog = Arc::new(FileSystemCatalog::new(options).unwrap()); + let typed_catalog = Arc::new(TypedTestCatalog { + inner: fs_catalog, + declared_types: HashMap::from([("it".to_string(), TableType::IcebergTable)]), + }); + let mut ctx = SQLContext::new(); + ctx.register_catalog(CATALOG, typed_catalog).await.unwrap(); + ctx.sql(&format!("CREATE SCHEMA {CATALOG}.{DB}")) + .await + .unwrap(); + + let Err(err) = ctx.sql(&format!("SELECT * FROM {CATALOG}.{DB}.it")).await else { + panic!("an external table without an engine must not resolve"); + }; + let msg = err.to_string(); + assert!(msg.contains("no table engine is registered"), "{msg}"); + assert!(msg.contains("iceberg-table"), "{msg}"); +} diff --git a/crates/paimon-rest-server/tests/e2e.rs b/crates/paimon-rest-server/tests/e2e.rs index 94f8fa277..db27bb3eb 100644 --- a/crates/paimon-rest-server/tests/e2e.rs +++ b/crates/paimon-rest-server/tests/e2e.rs @@ -432,9 +432,8 @@ async fn test_special_char_names() { #[tokio::test] async fn declared_engine_type_routes_through_the_rest_catalog() { - use paimon::catalog::RoutedTableLoad; + use paimon::catalog::LoadedTable; use paimon::spec::TableType; - use std::collections::HashSet; let ctx = setup().await; ctx.catalog @@ -455,11 +454,11 @@ async fn declared_engine_type_routes_through_the_rest_catalog() { let routed = ctx .catalog - .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) + .load_table(&identifier) .await .expect("routing must reach the declared type"); assert!( - matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), + matches!(routed, LoadedTable::External(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); diff --git a/crates/paimon/src/catalog/filesystem.rs b/crates/paimon/src/catalog/filesystem.rs index d759cd576..0ebd8ab84 100644 --- a/crates/paimon/src/catalog/filesystem.rs +++ b/crates/paimon/src/catalog/filesystem.rs @@ -197,7 +197,6 @@ impl FileSystemCatalog { Ok((table_path, schema)) } - /// Build a Table from an already-fetched schema. fn build_table( &self, identifier: &Identifier, @@ -357,25 +356,19 @@ impl Catalog for FileSystemCatalog { self.build_table(identifier, table_path, schema) } - async fn load_table_routing( - &self, - identifier: &Identifier, - engine_types: &std::collections::HashSet, - ) -> Result { + async fn load_table(&self, identifier: &Identifier) -> Result { let (table_path, schema) = self.fetch_table_schema(identifier).await?; let options = CoreOptions::new(schema.options()); let declared = options.table_type()?; - if engine_types.contains(&declared) { - // Neither this client nor an engine can enforce the - // server-side row filter / column mask. - return crate::catalog::RoutedTableLoad::engine( + if declared.requires_table_engine() { + return crate::catalog::LoadedTable::external( declared, &options, &identifier.full_name(), ); } self.build_table(identifier, table_path, schema) - .map(|table| crate::catalog::RoutedTableLoad::Paimon(Box::new(table))) + .map(|table| crate::catalog::LoadedTable::Paimon(Box::new(table))) } async fn list_tables(&self, database_name: &str) -> Result> { @@ -836,8 +829,7 @@ mod tests { #[tokio::test] async fn test_stored_scan_selectors_block_routing() { - use crate::catalog::RoutedTableLoad; - use std::collections::HashSet; + use crate::catalog::LoadedTable; let (_temp_dir, catalog) = create_test_catalog(); catalog @@ -865,10 +857,7 @@ mod tests { .await .unwrap(); - let err = catalog - .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) - .await - .unwrap_err(); + let err = catalog.load_table(&identifier).await.unwrap_err(); assert!(matches!(err, Error::Unsupported { .. }), "{key}: {err:?}"); } @@ -885,16 +874,13 @@ mod tests { .create_table(&identifier, schema, false) .await .unwrap(); - let routed = catalog - .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) - .await - .unwrap(); - assert!(matches!(routed, RoutedTableLoad::Engine(_)), "{routed:?}"); + let routed = catalog.load_table(&identifier).await.unwrap(); + assert!(matches!(routed, LoadedTable::External(_)), "{routed:?}"); } #[tokio::test] async fn test_routing_cannot_skip_the_read_guards() { - use crate::catalog::RoutedTableLoad; + use crate::catalog::LoadedTable; use crate::spec::CoreOptions; // The only constructor of the `Engine` variant, so a third-party @@ -905,7 +891,7 @@ mod tests { ("incremental-between", "1,5"), ] { let options = HashMap::from([(key.to_string(), value.to_string())]); - let err = RoutedTableLoad::engine( + let err = LoadedTable::external( TableType::IcebergTable, &CoreOptions::new(&options), "db1.t", @@ -915,7 +901,7 @@ mod tests { } let options = HashMap::new(); - assert!(RoutedTableLoad::engine( + assert!(LoadedTable::external( TableType::IcebergTable, &CoreOptions::new(&options), "db1.t", @@ -957,8 +943,7 @@ mod tests { #[tokio::test] async fn test_declared_engine_type_routes_and_fails_closed() { - use crate::catalog::RoutedTableLoad; - use std::collections::HashSet; + use crate::catalog::LoadedTable; let (_temp_dir, catalog) = create_test_catalog(); catalog @@ -979,12 +964,9 @@ mod tests { .await .unwrap(); - let routed = catalog - .load_table_routing(&identifier, &HashSet::from([TableType::IcebergTable])) - .await - .unwrap(); + let routed = catalog.load_table(&identifier).await.unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), + matches!(routed, LoadedTable::External(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); @@ -994,12 +976,6 @@ mod tests { if message.contains("cannot be read as a Paimon table")), "{err:?}" ); - - let err = catalog - .load_table_routing(&identifier, &HashSet::new()) - .await - .unwrap_err(); - assert!(matches!(err, Error::Unsupported { .. }), "{err:?}"); } #[tokio::test] diff --git a/crates/paimon/src/catalog/mod.rs b/crates/paimon/src/catalog/mod.rs index a0472cdcf..b2a5cf232 100644 --- a/crates/paimon/src/catalog/mod.rs +++ b/crates/paimon/src/catalog/mod.rs @@ -265,41 +265,51 @@ use crate::api::PagedList; use crate::spec::{Partition, Schema, SchemaChange, TableType}; use crate::table::Table; -/// Outcome of [`Catalog::load_table_routing`]. +/// Outcome of [`Catalog::load_table`]. #[derive(Debug)] -pub enum RoutedTableLoad { +pub enum LoadedTable { /// A constructed Paimon table (boxed: far larger than the other variant). Paimon(Box
), - /// The declared type is in `engine_types`; construction was skipped. - Engine(EngineTable), + /// A table this reader cannot construct. + External(ExternalTableMetadata), } -/// A table handed to a registered engine. Only [`RoutedTableLoad::engine`] -/// can build one, so no catalog — including a third-party implementation — -/// routes a table without the checks that constructor performs. +/// What a caller needs to pick an engine for a table Paimon cannot construct. +/// Only [`LoadedTable::external`] can build one, so the stored-metadata checks +/// always run before a caller sees it. #[derive(Debug)] -pub struct EngineTable { +pub struct ExternalTableMetadata { declared: TableType, } -impl EngineTable { +impl ExternalTableMetadata { /// The type the table's metadata declares. pub fn declared(&self) -> TableType { self.declared } } -impl RoutedTableLoad { - /// Route `full_name` to the engine registered for `declared`, refusing - /// what [`CoreOptions::ensure_engine_can_serve`](crate::spec::CoreOptions::ensure_engine_can_serve) - /// refuses. - pub fn engine( +impl LoadedTable { + /// Classify `full_name` as external, refusing options no engine can honor + /// and reads this client cannot authorize. + /// + /// Errors if [`TableType::requires_table_engine`] rejects `declared`: + /// classifying a Paimon-served type as external would skip the reader that + /// can actually construct it. + pub fn external( declared: TableType, options: &crate::spec::CoreOptions<'_>, full_name: &str, ) -> Result { + if !declared.requires_table_engine() { + return Err(Error::Unsupported { + message: format!( + "table '{full_name}' is declared '{declared}', which the Paimon reader serves" + ), + }); + } options.ensure_engine_can_serve(full_name)?; - Ok(Self::Engine(EngineTable { declared })) + Ok(Self::External(ExternalTableMetadata { declared })) } } @@ -361,20 +371,16 @@ pub trait Catalog: Send + Sync { /// * [`crate::Error::TableNotExist`] - table does not exist. async fn get_table(&self, identifier: &Identifier) -> Result
; - /// Load a table, or return only its declared [`TableType`] when that - /// type is in `engine_types`, skipping construction and its token/FileIO - /// I/O. One metadata round-trip either way. The default implementation - /// always constructs, for catalogs without a table-type concept. + /// Load a table, or classify it as [`LoadedTable::External`] when this + /// reader cannot construct it. One metadata round-trip either way, and the + /// outcome depends only on the table's own metadata. The default + /// implementation always constructs, for catalogs without a table-type + /// concept. /// /// # Errors /// Same as [`Catalog::get_table`]. - async fn load_table_routing( - &self, - identifier: &Identifier, - engine_types: &std::collections::HashSet, - ) -> Result { - let _ = engine_types; - Ok(RoutedTableLoad::Paimon(Box::new( + async fn load_table(&self, identifier: &Identifier) -> Result { + Ok(LoadedTable::Paimon(Box::new( self.get_table(identifier).await?, ))) } diff --git a/crates/paimon/src/catalog/rest/rest_catalog.rs b/crates/paimon/src/catalog/rest/rest_catalog.rs index 352220705..be41cfc87 100644 --- a/crates/paimon/src/catalog/rest/rest_catalog.rs +++ b/crates/paimon/src/catalog/rest/rest_catalog.rs @@ -215,19 +215,13 @@ impl Catalog for RESTCatalog { .await } - async fn load_table_routing( - &self, - identifier: &Identifier, - engine_types: &std::collections::HashSet, - ) -> Result { + async fn load_table(&self, identifier: &Identifier) -> Result { let response = RESTEnv::fetch_table_response(identifier, &self.api).await?; if let Some(schema) = response.schema.as_ref() { let options = crate::spec::CoreOptions::new(schema.options()); let declared = options.table_type()?; - if engine_types.contains(&declared) { - // Neither this client nor an engine can enforce the - // server-side row filter / column mask. - return crate::catalog::RoutedTableLoad::engine( + if declared.requires_table_engine() { + return crate::catalog::LoadedTable::external( declared, &options, &identifier.full_name(), @@ -243,7 +237,7 @@ impl Catalog for RESTCatalog { self.local_cache.clone(), ) .await - .map(|table| crate::catalog::RoutedTableLoad::Paimon(Box::new(table))) + .map(|table| crate::catalog::LoadedTable::Paimon(Box::new(table))) } async fn list_tables(&self, database_name: &str) -> Result> { diff --git a/crates/paimon/src/spec/schema.rs b/crates/paimon/src/spec/schema.rs index 8ebd5c28b..0b52571b4 100644 --- a/crates/paimon/src/spec/schema.rs +++ b/crates/paimon/src/spec/schema.rs @@ -1155,8 +1155,7 @@ impl Schema { primary_keys: &[String], options: &HashMap, ) -> crate::Result<()> { - // Shared by create (`Schema::new`) and alter (`apply_changes`), so an - // unparsable type never reaches a metadata write. + // Create and alter share this, so an unparsable type never lands. CoreOptions::new(options).table_type()?; validate_no_reserved_field_names(fields)?; Self::validate_key_field_types(fields, primary_keys, options)?; diff --git a/crates/paimon/src/spec/table_type.rs b/crates/paimon/src/spec/table_type.rs index 37e61f57c..e4a095d23 100644 --- a/crates/paimon/src/spec/table_type.rs +++ b/crates/paimon/src/spec/table_type.rs @@ -54,10 +54,10 @@ impl TableType { } /// Whether this type needs an engine of its own (see - /// [`Catalog::load_table_routing`](crate::catalog::Catalog::load_table_routing)). - /// Java builds a dedicated table for each; this client has none, so - /// reading one as Paimon misreads it and writing could put Paimon - /// snapshots over foreign data. + /// [`Catalog::load_table`](crate::catalog::Catalog::load_table)). Java + /// builds a dedicated table for each; this client has none, so reading one + /// as Paimon misreads it and writing could put Paimon snapshots over + /// foreign data. pub fn requires_table_engine(&self) -> bool { matches!( self, diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index d94d0f6bd..83da7c8a7 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -1503,10 +1503,9 @@ async fn test_catalog_maps_unsupported_view_and_function_endpoints() { } #[tokio::test] -async fn test_load_table_routing_returns_engine_for_declared_type() { - use paimon::catalog::RoutedTableLoad; +async fn test_load_table_returns_external_for_declared_type() { + use paimon::catalog::LoadedTable; use paimon::spec::TableType; - use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; let schema = Schema::builder() @@ -1518,29 +1517,15 @@ async fn test_load_table_routing_returns_engine_for_declared_type() { .add_table_with_schema("default", "ice_t", schema, "file:///unused"); let identifier = Identifier::new("default", "ice_t"); - let engine_types = HashSet::from([TableType::IcebergTable]); - let routed = ctx - .catalog - .load_table_routing(&identifier, &engine_types) - .await - .unwrap(); + let routed = ctx.catalog.load_table(&identifier).await.unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), + matches!(routed, LoadedTable::External(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); - - let routed = ctx - .catalog - .load_table_routing(&identifier, &HashSet::new()) - .await; - assert!(!matches!(routed, Ok(RoutedTableLoad::Engine(_)))); } #[tokio::test] -async fn test_load_table_routing_fails_closed_on_query_auth() { - use paimon::spec::TableType; - use std::collections::HashSet; - +async fn test_load_table_fails_closed_on_query_auth() { let ctx = setup_catalog(vec!["default"]).await; let schema = Schema::builder() .column("id", DataType::BigInt(BigIntType::new())) @@ -1552,12 +1537,7 @@ async fn test_load_table_routing_fails_closed_on_query_auth() { .add_table_with_schema("default", "authed_ice_t", schema, "file:///unused"); let identifier = Identifier::new("default", "authed_ice_t"); - let engine_types = HashSet::from([TableType::IcebergTable]); - let err = ctx - .catalog - .load_table_routing(&identifier, &engine_types) - .await - .unwrap_err(); + let err = ctx.catalog.load_table(&identifier).await.unwrap_err(); assert!( matches!(err, paimon::Error::Unsupported { ref message } if message.contains("query-auth")), "{err:?}" @@ -1565,10 +1545,8 @@ async fn test_load_table_routing_fails_closed_on_query_auth() { } #[tokio::test] -async fn test_load_table_routing_constructs_paimon_for_undeclared_type() { - use paimon::catalog::RoutedTableLoad; - use paimon::spec::TableType; - use std::collections::HashSet; +async fn test_load_table_constructs_paimon_for_undeclared_type() { + use paimon::catalog::LoadedTable; let ctx = setup_catalog(vec!["default"]).await; let schema = Schema::builder() @@ -1579,14 +1557,9 @@ async fn test_load_table_routing_constructs_paimon_for_undeclared_type() { .add_table_with_schema("default", "plain_t", schema, "file:///unused"); let identifier = Identifier::new("default", "plain_t"); - let engine_types = HashSet::from([TableType::IcebergTable]); - let routed = ctx - .catalog - .load_table_routing(&identifier, &engine_types) - .await - .unwrap(); + let routed = ctx.catalog.load_table(&identifier).await.unwrap(); match routed { - RoutedTableLoad::Paimon(table) => { + LoadedTable::Paimon(table) => { assert_eq!(table.identifier().full_name(), "default.plain_t"); } other => panic!("expected a constructed Paimon table, got {other:?}"), @@ -1617,10 +1590,9 @@ async fn test_get_table_fails_closed_on_iceberg_table() { } #[tokio::test] -async fn test_load_table_routing_parses_declared_type_case_insensitively() { - use paimon::catalog::RoutedTableLoad; +async fn test_load_table_parses_declared_type_case_insensitively() { + use paimon::catalog::LoadedTable; use paimon::spec::TableType; - use std::collections::HashSet; let ctx = setup_catalog(vec!["default"]).await; let schema = Schema::builder() @@ -1633,22 +1605,17 @@ async fn test_load_table_routing_parses_declared_type_case_insensitively() { let routed = ctx .catalog - .load_table_routing( - &Identifier::new("default", "upper_ice_t"), - &HashSet::from([TableType::IcebergTable]), - ) + .load_table(&Identifier::new("default", "upper_ice_t")) .await .unwrap(); assert!( - matches!(routed, RoutedTableLoad::Engine(ref e) if e.declared() == TableType::IcebergTable), + matches!(routed, LoadedTable::External(ref e) if e.declared() == TableType::IcebergTable), "{routed:?}" ); } #[tokio::test] -async fn test_load_table_routing_rejects_unknown_declared_type() { - use std::collections::HashSet; - +async fn test_load_table_rejects_unknown_declared_type() { let ctx = setup_catalog(vec!["default"]).await; let valid = Schema::builder() .column("id", DataType::BigInt(BigIntType::new())) @@ -1662,7 +1629,7 @@ async fn test_load_table_routing_rejects_unknown_declared_type() { let err = ctx .catalog - .load_table_routing(&Identifier::new("default", "delta_t"), &HashSet::new()) + .load_table(&Identifier::new("default", "delta_t")) .await .unwrap_err(); assert!(