From 20bafe01de9b9ead7016d32afa8a33f75d868618 Mon Sep 17 00:00:00 2001 From: beinan Date: Thu, 12 Feb 2026 08:57:21 +0000 Subject: [PATCH 1/2] refactor: remove catalog re-export modules and update imports Remove redundant re-export modules (namespace/directory.rs and source_catalog.rs) and update all imports to use lance_graph_catalog directly. This addresses code review feedback from PR #130. Changes: - Remove crates/lance-graph/src/namespace/ directory - Remove crates/lance-graph/src/source_catalog.rs - Update lib.rs to re-export catalog types from lance_graph_catalog - Update all internal imports to use lance_graph_catalog - Update Python bindings to use new import paths Co-Authored-By: Claude Opus 4.6 --- .../src/datafusion_planner/config_helpers.rs | 2 +- .../src/datafusion_planner/join_ops.rs | 2 +- .../lance-graph/src/datafusion_planner/mod.rs | 2 +- .../src/datafusion_planner/scan_ops.rs | 4 ++-- .../src/datafusion_planner/test_fixtures.rs | 4 ++-- crates/lance-graph/src/lib.rs | 4 +--- crates/lance-graph/src/namespace/directory.rs | 3 --- crates/lance-graph/src/namespace/mod.rs | 3 --- crates/lance-graph/src/query.rs | 22 +++++++++---------- crates/lance-graph/src/source_catalog.rs | 4 ---- python/src/graph.rs | 5 ++--- python/src/namespace.rs | 2 +- 12 files changed, 22 insertions(+), 35 deletions(-) delete mode 100644 crates/lance-graph/src/namespace/directory.rs delete mode 100644 crates/lance-graph/src/namespace/mod.rs delete mode 100644 crates/lance-graph/src/source_catalog.rs diff --git a/crates/lance-graph/src/datafusion_planner/config_helpers.rs b/crates/lance-graph/src/datafusion_planner/config_helpers.rs index c8172b7..fd94433 100644 --- a/crates/lance-graph/src/datafusion_planner/config_helpers.rs +++ b/crates/lance-graph/src/datafusion_planner/config_helpers.rs @@ -10,7 +10,7 @@ use super::analysis::PlanningContext; use super::DataFusionPlanner; use crate::config::{NodeMapping, RelationshipMapping}; use crate::error::Result; -use crate::source_catalog::GraphSourceCatalog; +use lance_graph_catalog::GraphSourceCatalog; use std::sync::Arc; impl DataFusionPlanner { diff --git a/crates/lance-graph/src/datafusion_planner/join_ops.rs b/crates/lance-graph/src/datafusion_planner/join_ops.rs index ef815ef..bd3afff 100644 --- a/crates/lance-graph/src/datafusion_planner/join_ops.rs +++ b/crates/lance-graph/src/datafusion_planner/join_ops.rs @@ -14,7 +14,7 @@ use crate::ast::{PropertyValue, RelationshipDirection}; use crate::case_insensitive::qualify_column; use crate::config::{NodeMapping, RelationshipMapping}; use crate::error::Result; -use crate::source_catalog::GraphSourceCatalog; +use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::{ col, BinaryExpr, Expr, JoinType, LogicalPlan, LogicalPlanBuilder, Operator, TableSource, }; diff --git a/crates/lance-graph/src/datafusion_planner/mod.rs b/crates/lance-graph/src/datafusion_planner/mod.rs index a5387f9..b0ccc07 100644 --- a/crates/lance-graph/src/datafusion_planner/mod.rs +++ b/crates/lance-graph/src/datafusion_planner/mod.rs @@ -32,7 +32,7 @@ pub use analysis::{PlanningContext, QueryAnalysis, RelationshipInstance}; use crate::config::GraphConfig; use crate::error::Result; use crate::logical_plan::LogicalOperator; -use crate::source_catalog::GraphSourceCatalog; +use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::LogicalPlan; use std::sync::Arc; diff --git a/crates/lance-graph/src/datafusion_planner/scan_ops.rs b/crates/lance-graph/src/datafusion_planner/scan_ops.rs index 912f07d..ceca0e5 100644 --- a/crates/lance-graph/src/datafusion_planner/scan_ops.rs +++ b/crates/lance-graph/src/datafusion_planner/scan_ops.rs @@ -9,7 +9,7 @@ use super::analysis::{PlanningContext, RelationshipInstance}; use crate::ast::PropertyValue; use crate::case_insensitive::qualify_column; use crate::error::Result; -use crate::source_catalog::GraphSourceCatalog; +use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::{col, BinaryExpr, Expr, LogicalPlan, LogicalPlanBuilder, Operator}; use std::collections::{HashMap, HashSet}; use std::sync::Arc; @@ -107,7 +107,7 @@ impl DataFusionPlanner { // No catalog attached - create empty source fallback for flexibility // This allows planners created with DataFusionPlanner::new() to work // without requiring a catalog, though they won't have actual data sources - let empty_source = Arc::new(crate::source_catalog::SimpleTableSource::empty()); + let empty_source = Arc::new(lance_graph_catalog::SimpleTableSource::empty()); let normalized_label = label.to_lowercase(); let builder = LogicalPlanBuilder::scan(&normalized_label, empty_source, None).map_err(|e| { diff --git a/crates/lance-graph/src/datafusion_planner/test_fixtures.rs b/crates/lance-graph/src/datafusion_planner/test_fixtures.rs index b586c52..c65710b 100644 --- a/crates/lance-graph/src/datafusion_planner/test_fixtures.rs +++ b/crates/lance-graph/src/datafusion_planner/test_fixtures.rs @@ -5,7 +5,7 @@ use crate::config::GraphConfig; use crate::logical_plan::LogicalOperator; -use crate::source_catalog::{InMemoryCatalog, SimpleTableSource}; +use lance_graph_catalog::{InMemoryCatalog, SimpleTableSource}; use arrow_schema::{DataType, Field, Schema}; use std::sync::Arc; @@ -17,7 +17,7 @@ pub fn person_schema() -> Arc { ])) } -pub fn make_catalog() -> Arc { +pub fn make_catalog() -> Arc { let person_src = Arc::new(SimpleTableSource::new(person_schema())); let knows_schema = Arc::new(Schema::new(vec![ Field::new("src_person_id", DataType::Int64, false), diff --git a/crates/lance-graph/src/lib.rs b/crates/lance-graph/src/lib.rs index cba6d6f..e83df06 100644 --- a/crates/lance-graph/src/lib.rs +++ b/crates/lance-graph/src/lib.rs @@ -43,18 +43,16 @@ pub mod error; pub mod lance_native_planner; pub mod lance_vector_search; pub mod logical_plan; -pub mod namespace; pub mod parser; pub mod query; pub mod semantic; pub mod simple_executor; -pub mod source_catalog; /// Maximum allowed hops for variable-length relationship expansion (e.g., *1..N) pub const MAX_VARIABLE_LENGTH_HOPS: u32 = 20; pub use config::{GraphConfig, NodeMapping, RelationshipMapping}; pub use error::{GraphError, Result}; +pub use lance_graph_catalog::{DirNamespace, GraphSourceCatalog, InMemoryCatalog, SimpleTableSource}; pub use lance_vector_search::VectorSearch; -pub use namespace::DirNamespace; pub use query::{CypherQuery, ExecutionStrategy}; diff --git a/crates/lance-graph/src/namespace/directory.rs b/crates/lance-graph/src/namespace/directory.rs deleted file mode 100644 index a094a56..0000000 --- a/crates/lance-graph/src/namespace/directory.rs +++ /dev/null @@ -1,3 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -pub use lance_graph_catalog::DirNamespace; diff --git a/crates/lance-graph/src/namespace/mod.rs b/crates/lance-graph/src/namespace/mod.rs deleted file mode 100644 index fe6c89c..0000000 --- a/crates/lance-graph/src/namespace/mod.rs +++ /dev/null @@ -1,3 +0,0 @@ -pub mod directory; - -pub use directory::DirNamespace; diff --git a/crates/lance-graph/src/query.rs b/crates/lance-graph/src/query.rs index 8f5f017..cf0f974 100644 --- a/crates/lance-graph/src/query.rs +++ b/crates/lance-graph/src/query.rs @@ -8,7 +8,7 @@ use crate::ast::ReadingClause; use crate::config::GraphConfig; use crate::error::{GraphError, Result}; use crate::logical_plan::LogicalPlanner; -use crate::namespace::DirNamespace; +use lance_graph_catalog::DirNamespace; use crate::parser::parse_cypher_query; use crate::simple_executor::{ to_df_boolean_expr_simple, to_df_order_by_expr_simple, to_df_value_expr_simple, PathExecutor, @@ -395,7 +395,7 @@ impl CypherQuery { &self, ctx: datafusion::execution::context::SessionContext, ) -> Result { - use crate::source_catalog::InMemoryCatalog; + use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::DefaultTableSource; use std::sync::Arc; @@ -463,7 +463,7 @@ impl CypherQuery { /// ```ignore /// use std::sync::Arc; /// use datafusion::execution::context::SessionContext; - /// use lance_graph::source_catalog::InMemoryCatalog; + /// use lance_graph::InMemoryCatalog; /// use lance_graph::query::CypherQuery; /// /// // Create custom catalog @@ -481,7 +481,7 @@ impl CypherQuery { /// ``` pub async fn execute_with_catalog_and_context( &self, - catalog: std::sync::Arc, + catalog: std::sync::Arc, ctx: datafusion::execution::context::SessionContext, ) -> Result { use arrow::compute::concat_batches; @@ -557,10 +557,10 @@ impl CypherQuery { &self, datasets: HashMap, ) -> Result<( - crate::source_catalog::InMemoryCatalog, + lance_graph_catalog::InMemoryCatalog, datafusion::execution::context::SessionContext, )> { - use crate::source_catalog::InMemoryCatalog; + use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::{DefaultTableSource, MemTable}; use datafusion::execution::context::SessionContext; use std::sync::Arc; @@ -619,10 +619,10 @@ impl CypherQuery { &self, namespace: std::sync::Arc, ) -> Result<( - crate::source_catalog::InMemoryCatalog, + lance_graph_catalog::InMemoryCatalog, datafusion::execution::context::SessionContext, )> { - use crate::source_catalog::InMemoryCatalog; + use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::{DefaultTableSource, TableProvider}; use datafusion::execution::context::SessionContext; use lance::datafusion::LanceTableProvider; @@ -740,7 +740,7 @@ impl CypherQuery { /// Internal helper to explain the query execution plan with explicit catalog and session context async fn explain_internal( &self, - catalog: std::sync::Arc, + catalog: std::sync::Arc, ctx: datafusion::execution::context::SessionContext, ) -> Result { // Create all plans (phases 1-4) @@ -757,7 +757,7 @@ impl CypherQuery { /// DataFusion logical planning) without creating the physical plan. fn create_logical_plans( &self, - catalog: std::sync::Arc, + catalog: std::sync::Arc, ) -> Result<( crate::logical_plan::LogicalOperator, datafusion::logical_expr::LogicalPlan, @@ -791,7 +791,7 @@ impl CypherQuery { /// Helper to create all plans (graph logical, DataFusion logical, physical) async fn create_plans( &self, - catalog: std::sync::Arc, + catalog: std::sync::Arc, ctx: &datafusion::execution::context::SessionContext, ) -> Result<( crate::logical_plan::LogicalOperator, diff --git a/crates/lance-graph/src/source_catalog.rs b/crates/lance-graph/src/source_catalog.rs deleted file mode 100644 index bf53303..0000000 --- a/crates/lance-graph/src/source_catalog.rs +++ /dev/null @@ -1,4 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright The Lance Authors - -pub use lance_graph_catalog::{GraphSourceCatalog, InMemoryCatalog, SimpleTableSource}; diff --git a/python/src/graph.rs b/python/src/graph.rs index 584553d..f6a7380 100644 --- a/python/src/graph.rs +++ b/python/src/graph.rs @@ -26,9 +26,8 @@ use datafusion::execution::context::SessionContext; use lance_graph::{ ast::DistanceMetric as RustDistanceMetric, CypherQuery as RustCypherQuery, ExecutionStrategy as RustExecutionStrategy, GraphConfig as RustGraphConfig, - GraphError as RustGraphError, VectorSearch as RustVectorSearch, + GraphError as RustGraphError, VectorSearch as RustVectorSearch, InMemoryCatalog, }; -use lance_graph::source_catalog::InMemoryCatalog; use pyo3::{ exceptions::{PyNotImplementedError, PyRuntimeError, PyValueError}, prelude::*, @@ -919,7 +918,7 @@ fn record_batch_to_python_table( #[pyclass(name = "CypherEngine", module = "lance.graph")] pub struct CypherEngine { config: RustGraphConfig, - catalog: Arc, + catalog: Arc, context: Arc, } diff --git a/python/src/namespace.rs b/python/src/namespace.rs index 491a6f0..e8af4ca 100644 --- a/python/src/namespace.rs +++ b/python/src/namespace.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use lance_graph::namespace::DirNamespace; +use lance_graph::DirNamespace; use pyo3::prelude::*; #[pyclass(name = "DirNamespace", module = "lance.graph")] From fb7c693adf2b1c76984155a00c4f5351aa6c6be8 Mon Sep 17 00:00:00 2001 From: beinan Date: Thu, 12 Feb 2026 09:03:32 +0000 Subject: [PATCH 2/2] style: apply cargo fmt Co-Authored-By: Claude Opus 4.6 --- crates/lance-graph/src/datafusion_planner/join_ops.rs | 2 +- crates/lance-graph/src/datafusion_planner/mod.rs | 2 +- crates/lance-graph/src/datafusion_planner/scan_ops.rs | 2 +- .../lance-graph/src/datafusion_planner/test_fixtures.rs | 2 +- crates/lance-graph/src/lib.rs | 4 +++- crates/lance-graph/src/query.rs | 8 ++++---- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/crates/lance-graph/src/datafusion_planner/join_ops.rs b/crates/lance-graph/src/datafusion_planner/join_ops.rs index bd3afff..d88a1da 100644 --- a/crates/lance-graph/src/datafusion_planner/join_ops.rs +++ b/crates/lance-graph/src/datafusion_planner/join_ops.rs @@ -14,10 +14,10 @@ use crate::ast::{PropertyValue, RelationshipDirection}; use crate::case_insensitive::qualify_column; use crate::config::{NodeMapping, RelationshipMapping}; use crate::error::Result; -use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::{ col, BinaryExpr, Expr, JoinType, LogicalPlan, LogicalPlanBuilder, Operator, TableSource, }; +use lance_graph_catalog::GraphSourceCatalog; use std::collections::HashMap; use std::sync::Arc; diff --git a/crates/lance-graph/src/datafusion_planner/mod.rs b/crates/lance-graph/src/datafusion_planner/mod.rs index b0ccc07..dfb4bb5 100644 --- a/crates/lance-graph/src/datafusion_planner/mod.rs +++ b/crates/lance-graph/src/datafusion_planner/mod.rs @@ -32,8 +32,8 @@ pub use analysis::{PlanningContext, QueryAnalysis, RelationshipInstance}; use crate::config::GraphConfig; use crate::error::Result; use crate::logical_plan::LogicalOperator; -use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::LogicalPlan; +use lance_graph_catalog::GraphSourceCatalog; use std::sync::Arc; /// Planner abstraction for graph-to-physical planning diff --git a/crates/lance-graph/src/datafusion_planner/scan_ops.rs b/crates/lance-graph/src/datafusion_planner/scan_ops.rs index ceca0e5..200ae9a 100644 --- a/crates/lance-graph/src/datafusion_planner/scan_ops.rs +++ b/crates/lance-graph/src/datafusion_planner/scan_ops.rs @@ -9,8 +9,8 @@ use super::analysis::{PlanningContext, RelationshipInstance}; use crate::ast::PropertyValue; use crate::case_insensitive::qualify_column; use crate::error::Result; -use lance_graph_catalog::GraphSourceCatalog; use datafusion::logical_expr::{col, BinaryExpr, Expr, LogicalPlan, LogicalPlanBuilder, Operator}; +use lance_graph_catalog::GraphSourceCatalog; use std::collections::{HashMap, HashSet}; use std::sync::Arc; diff --git a/crates/lance-graph/src/datafusion_planner/test_fixtures.rs b/crates/lance-graph/src/datafusion_planner/test_fixtures.rs index c65710b..7aceba6 100644 --- a/crates/lance-graph/src/datafusion_planner/test_fixtures.rs +++ b/crates/lance-graph/src/datafusion_planner/test_fixtures.rs @@ -5,8 +5,8 @@ use crate::config::GraphConfig; use crate::logical_plan::LogicalOperator; -use lance_graph_catalog::{InMemoryCatalog, SimpleTableSource}; use arrow_schema::{DataType, Field, Schema}; +use lance_graph_catalog::{InMemoryCatalog, SimpleTableSource}; use std::sync::Arc; pub fn person_schema() -> Arc { diff --git a/crates/lance-graph/src/lib.rs b/crates/lance-graph/src/lib.rs index e83df06..692773a 100644 --- a/crates/lance-graph/src/lib.rs +++ b/crates/lance-graph/src/lib.rs @@ -53,6 +53,8 @@ pub const MAX_VARIABLE_LENGTH_HOPS: u32 = 20; pub use config::{GraphConfig, NodeMapping, RelationshipMapping}; pub use error::{GraphError, Result}; -pub use lance_graph_catalog::{DirNamespace, GraphSourceCatalog, InMemoryCatalog, SimpleTableSource}; +pub use lance_graph_catalog::{ + DirNamespace, GraphSourceCatalog, InMemoryCatalog, SimpleTableSource, +}; pub use lance_vector_search::VectorSearch; pub use query::{CypherQuery, ExecutionStrategy}; diff --git a/crates/lance-graph/src/query.rs b/crates/lance-graph/src/query.rs index cf0f974..0a10e7f 100644 --- a/crates/lance-graph/src/query.rs +++ b/crates/lance-graph/src/query.rs @@ -8,13 +8,13 @@ use crate::ast::ReadingClause; use crate::config::GraphConfig; use crate::error::{GraphError, Result}; use crate::logical_plan::LogicalPlanner; -use lance_graph_catalog::DirNamespace; use crate::parser::parse_cypher_query; use crate::simple_executor::{ to_df_boolean_expr_simple, to_df_order_by_expr_simple, to_df_value_expr_simple, PathExecutor, }; use arrow_array::RecordBatch; use arrow_schema::{Field, Schema, SchemaRef}; +use lance_graph_catalog::DirNamespace; use lance_namespace::models::DescribeTableRequest; use std::collections::{HashMap, HashSet}; use std::sync::Arc; @@ -395,8 +395,8 @@ impl CypherQuery { &self, ctx: datafusion::execution::context::SessionContext, ) -> Result { - use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::DefaultTableSource; + use lance_graph_catalog::InMemoryCatalog; use std::sync::Arc; let config = self.require_config()?; @@ -560,9 +560,9 @@ impl CypherQuery { lance_graph_catalog::InMemoryCatalog, datafusion::execution::context::SessionContext, )> { - use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::{DefaultTableSource, MemTable}; use datafusion::execution::context::SessionContext; + use lance_graph_catalog::InMemoryCatalog; use std::sync::Arc; if datasets.is_empty() { @@ -622,10 +622,10 @@ impl CypherQuery { lance_graph_catalog::InMemoryCatalog, datafusion::execution::context::SessionContext, )> { - use lance_graph_catalog::InMemoryCatalog; use datafusion::datasource::{DefaultTableSource, TableProvider}; use datafusion::execution::context::SessionContext; use lance::datafusion::LanceTableProvider; + use lance_graph_catalog::InMemoryCatalog; use std::sync::Arc; let config = self.require_config()?;