luoyuxia commented on code in PR #293:
URL: https://github.com/apache/paimon-rust/pull/293#discussion_r3166898990
##########
crates/integrations/datafusion/src/sql_context.rs:
##########
@@ -1528,8 +1611,10 @@ mod tests {
}
}
- fn make_handler(catalog: Arc<MockCatalog>) -> PaimonSqlHandler {
- PaimonSqlHandler::new(SessionContext::new(), catalog,
"paimon").unwrap()
+ fn make_handler(catalog: Arc<MockCatalog>) -> SQLContext {
Review Comment:
nit:
make_sql_context?
##########
docs/src/datafusion.md:
##########
@@ -717,13 +715,13 @@ async fn main() -> Result<(), Box<dyn std::error::Error>>
{
options.set(CatalogOptions::WAREHOUSE, "file:///tmp/paimon-warehouse");
let catalog = Arc::new(FileSystemCatalog::new(options)?);
- // Create handler (registers catalog provider and relation planner
automatically)
- let ctx = SessionContext::new();
- let handler = PaimonSqlHandler::new(ctx, catalog, "paimon")?;
+ // Create SQL context and register catalog
+ let mut ctx = SQLContext::new();
Review Comment:
we recommend Paimon own SQLContext as the entrypoint in rust, but python doc
are still using datafusion's context. Should be update
`bindings/python/project-description.md`, and `bindings/python/README.md` to
also use Paimon own SQLContext to make it unified?
##########
crates/integrations/datafusion/tests/merge_into_tests.rs:
##########
@@ -42,12 +41,13 @@ fn create_test_env() -> (TempDir, Arc<FileSystemCatalog>) {
(temp_dir, Arc::new(catalog))
}
-fn create_handler(catalog: Arc<FileSystemCatalog>) -> PaimonSqlHandler {
- let ctx = SessionContext::new();
- PaimonSqlHandler::new(ctx, catalog, "paimon").unwrap()
+fn create_handler(catalog: Arc<FileSystemCatalog>) -> SQLContext {
Review Comment:
nit:
may aslo rename to `create_sql_context`
##########
crates/integrations/datafusion/src/update.rs:
##########
@@ -335,17 +335,17 @@ mod tests {
use paimon::{CatalogOptions, FileSystemCatalog, Options};
use tempfile::TempDir;
- use crate::{PaimonSqlHandler, PaimonTableProvider};
+ use crate::{PaimonTableProvider, SQLContext};
- async fn setup_handler() -> (TempDir, PaimonSqlHandler,
Arc<FileSystemCatalog>) {
+ async fn setup_handler() -> (TempDir, SQLContext, Arc<FileSystemCatalog>) {
Review Comment:
may unify handler to sql_context to make it clear?
##########
crates/integrations/datafusion/src/sql_context.rs:
##########
@@ -654,34 +710,61 @@ impl PaimonSqlHandler {
ok_result(&self.ctx)
}
- /// Resolve an ObjectName like `paimon.db.table` or `db.table` to a Paimon
Identifier.
- fn resolve_table_name(&self, name: &ObjectName) -> DFResult<Identifier> {
+ fn current_catalog(&self) -> DFResult<Arc<dyn Catalog>> {
+ self.catalogs
+ .get(&self.current_catalog)
+ .cloned()
+ .ok_or_else(|| {
+ DataFusionError::Plan(
+ "No catalog registered. Call register_catalog()
first.".to_string(),
+ )
+ })
+ }
+
+ /// Resolve an ObjectName like `catalog.db.table` or `db.table` to a
catalog and Identifier.
+ fn resolve_catalog_and_table(
+ &self,
+ name: &ObjectName,
+ ) -> DFResult<(Arc<dyn Catalog>, String, Identifier)> {
let parts: Vec<String> = name
.0
.iter()
.filter_map(|p| p.as_ident().map(|id| id.value.clone()))
.collect();
match parts.len() {
3 => {
- // catalog.database.table — strip catalog prefix
- if parts[0] != self.catalog_name {
- return Err(DataFusionError::Plan(format!(
- "Unknown catalog '{}', expected '{}'",
- parts[0], self.catalog_name
- )));
- }
- Ok(Identifier::new(parts[1].clone(), parts[2].clone()))
+ let catalog = self.catalogs.get(&parts[0]).ok_or_else(|| {
+ DataFusionError::Plan(format!("Unknown catalog '{}'",
parts[0]))
+ })?;
+ Ok((
+ catalog.clone(),
+ parts[0].clone(),
+ Identifier::new(parts[1].clone(), parts[2].clone()),
+ ))
+ }
+ 2 => {
+ let catalog = self.current_catalog()?;
+ Ok((
+ catalog,
+ self.current_catalog.clone(),
+ Identifier::new(parts[0].clone(), parts[1].clone()),
+ ))
}
- 2 => Ok(Identifier::new(parts[0].clone(), parts[1].clone())),
1 => Err(DataFusionError::Plan(format!(
Review Comment:
just double check in here, is it expected that database name must be
specified since we already support set current database?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
