From 96598edac33e9c8dab8bf3b0ec1324308faf589e Mon Sep 17 00:00:00 2001 From: David Hughes Date: Sun, 25 Oct 2026 09:56:04 -0700 Subject: [PATCH 1/2] refactor(graph): add read-only config mapping iterators Migrate mapping readers while keeping public fields for source compatibility until a breaking release. Document migration and test normalized keys and original labels. Part of #23. Implemented with assistance from Codex; runtime and benchmark validation pending. --- crates/lance-graph-python/src/graph.rs | 26 ++++++---- crates/lance-graph/README.md | 7 +++ crates/lance-graph/src/config.rs | 51 ++++++++++++++----- .../builder/join_builder.rs | 6 +-- crates/lance-graph/src/query.rs | 19 ++++--- 5 files changed, 75 insertions(+), 34 deletions(-) diff --git a/crates/lance-graph-python/src/graph.rs b/crates/lance-graph-python/src/graph.rs index 778d688..813614d 100644 --- a/crates/lance-graph-python/src/graph.rs +++ b/crates/lance-graph-python/src/graph.rs @@ -348,13 +348,19 @@ impl GraphConfig { /// Get node labels fn node_labels(&self) -> Vec { // Extract from the config's node mappings - self.inner.node_mappings.keys().cloned().collect() + self.inner + .iter_node_mappings() + .map(|(name, _)| name.clone()) + .collect() } /// Get relationship types fn relationship_types(&self) -> Vec { // Extract from the config's relationship mappings - self.inner.relationship_mappings.keys().cloned().collect() + self.inner + .iter_relationship_mappings() + .map(|(name, _)| name.clone()) + .collect() } fn __repr__(&self) -> String { @@ -1437,10 +1443,10 @@ fn validate_sources( } Ok(()) }; - for mapping in config.node_mappings.values() { + for (_, mapping) in config.iter_node_mappings() { check(&mapping.label, &mapping.id_field)?; } - for mapping in config.relationship_mappings.values() { + for (_, mapping) in config.iter_relationship_mappings() { check(&mapping.relationship_type, &mapping.source_id_field)?; check(&mapping.relationship_type, &mapping.target_id_field)?; } @@ -1547,7 +1553,7 @@ fn build_node_index( previous: Option<&EngineState>, ) -> PyResult> { let mut entries = HashMap::new(); - for mapping in config.node_mappings.values() { + for (_, mapping) in config.iter_node_mappings() { let label = &mapping.label; let Some(source) = sources .iter() @@ -1657,7 +1663,7 @@ fn build_key_statistics( let mut to_count = Vec::new(); // Relationship type and endpoint columns, for the fan-out once counts are known. let mut endpoints = None; - for mapping in config.node_mappings.values() { + for (_, mapping) in config.iter_node_mappings() { if !mapping.label.eq_ignore_ascii_case(name) { continue; } @@ -1665,7 +1671,7 @@ fn build_key_statistics( distinct.insert(id.name().clone(), rows()?); } } - for mapping in config.relationship_mappings.values() { + for (_, mapping) in config.iter_relationship_mappings() { if !mapping.relationship_type.eq_ignore_ascii_case(name) { continue; } @@ -1764,7 +1770,7 @@ fn build_csr_entries( previous: Option<&EngineState>, ) -> PyResult> { let mut entries = HashMap::new(); - for mapping in config.relationship_mappings.values() { + for (_, mapping) in config.iter_relationship_mappings() { let rel_type = &mapping.relationship_type; let Some((name, source)) = sources .iter() @@ -2211,8 +2217,8 @@ impl CypherEngine { fn __repr__(&self) -> String { format!( "CypherEngine(nodes={}, relationships={})", - self.config.node_mappings.len(), - self.config.relationship_mappings.len() + self.config.iter_node_mappings().len(), + self.config.iter_relationship_mappings().len() ) } } diff --git a/crates/lance-graph/README.md b/crates/lance-graph/README.md index 5fecb3a..9750021 100644 --- a/crates/lance-graph/README.md +++ b/crates/lance-graph/README.md @@ -84,6 +84,13 @@ let config = GraphConfig::builder() .build()?; ``` +For Rust callers migrating away from direct mapping-field access, use `get_node_mapping(label)` and +`get_relationship_mapping(type_name)` for case-insensitive lookups. `iter_node_mappings()` and +`iter_relationship_mappings()` yield read-only `(normalized_key, mapping)` pairs; iteration order is +unspecified. Build mappings through `GraphConfigBuilder` so keys are normalized. Public fields remain +available for compatibility until a breaking release; replacing direct writes with builder calls avoids +that future migration. The serialized configuration and Python API are unchanged. + ## Executing Cypher Queries - `CypherQuery::new` parses Cypher text into the internal AST. diff --git a/crates/lance-graph/src/config.rs b/crates/lance-graph/src/config.rs index e35eb75..668791a 100644 --- a/crates/lance-graph/src/config.rs +++ b/crates/lance-graph/src/config.rs @@ -21,16 +21,8 @@ use std::collections::HashMap; /// # Future API Changes /// /// These fields may become private in a future major version to enforce invariants. -/// Code should migrate to using accessor methods (`get_node_mapping()`, `get_relationship_mapping()`) -/// rather than direct field access. -/// -/// # TODO: API Safety -/// -/// TODO: Make `node_mappings` and `relationship_mappings` private to prevent external code -/// from bypassing key normalization. This would require: -/// 1. Adding iterator methods for external access (e.g., `iter_node_mappings()`) -/// 2. Making this a breaking API change in next major version -/// 3. Ensuring all internal code uses accessor methods +/// Use the lookup and iterator accessors for reads, and [`GraphConfigBuilder`] for writes. +/// The mapping fields remain public for source compatibility until a breaking release. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct GraphConfig { /// Mapping of node labels to their field configurations @@ -126,6 +118,18 @@ impl GraphConfig { self.relationship_mappings.get(&rel_type.to_lowercase()) } + /// Iterate over normalized node labels and their mappings without allowing mutation. + pub fn iter_node_mappings(&self) -> impl ExactSizeIterator { + self.node_mappings.iter() + } + + /// Iterate over normalized relationship types and their mappings without allowing mutation. + pub fn iter_relationship_mappings( + &self, + ) -> impl ExactSizeIterator { + self.relationship_mappings.iter() + } + /// Validate the configuration /// /// Checks for: @@ -430,6 +434,29 @@ impl RelationshipMapping { mod tests { use super::*; + #[test] + fn test_mapping_iterators_keep_normalized_keys_and_original_labels() { + let config = GraphConfig::builder() + .node_table("Person", "id") + .rel_table("KNOWS", "Person", "Person", "src", "dst") + .build() + .unwrap(); + let (key, node) = config.iter_node_mappings().next().unwrap(); + assert_eq!(key, "person"); + assert_eq!(node.label, "Person"); + assert!(std::ptr::eq( + node, + config.get_node_mapping("PERSON").unwrap() + )); + let (key, relationship) = config.iter_relationship_mappings().next().unwrap(); + assert_eq!(key, "knows"); + assert_eq!(relationship.relationship_type, "KNOWS"); + assert!(std::ptr::eq( + relationship, + config.get_relationship_mapping("KnOwS").unwrap() + )); + } + #[test] fn test_graph_config_builder() { let config = GraphConfig::builder() @@ -439,8 +466,8 @@ mod tests { .build() .unwrap(); - assert_eq!(config.node_mappings.len(), 2); - assert_eq!(config.relationship_mappings.len(), 1); + assert_eq!(config.iter_node_mappings().len(), 2); + assert_eq!(config.iter_relationship_mappings().len(), 1); let person_mapping = config.get_node_mapping("Person").unwrap(); assert_eq!(person_mapping.id_field, "person_id"); diff --git a/crates/lance-graph/src/datafusion_planner/builder/join_builder.rs b/crates/lance-graph/src/datafusion_planner/builder/join_builder.rs index 7fc3ac3..97be618 100644 --- a/crates/lance-graph/src/datafusion_planner/builder/join_builder.rs +++ b/crates/lance-graph/src/datafusion_planner/builder/join_builder.rs @@ -242,10 +242,8 @@ impl DataFusionPlanner { .find(|r| r.alias == *var) { // Get the relationship mapping to find src/dst field names - if let Some(rel_map) = self - .config - .relationship_mappings - .get(&rel_instance.rel_type) + if let Some(rel_map) = + self.config.get_relationship_mapping(&rel_instance.rel_type) { // Generate composite join keys for both src_id and dst_id // This ensures we're matching the exact same relationship instance diff --git a/crates/lance-graph/src/query.rs b/crates/lance-graph/src/query.rs index 3b43c20..d7f890e 100644 --- a/crates/lance-graph/src/query.rs +++ b/crates/lance-graph/src/query.rs @@ -480,7 +480,7 @@ impl CypherQuery { let mut catalog = InMemoryCatalog::new(); // Register node sources - for label in config.node_mappings.keys() { + for (label, _) in config.iter_node_mappings() { let table_provider = ctx.table_provider(label) .await @@ -497,7 +497,7 @@ impl CypherQuery { } // Register relationship sources - for rel_type in config.relationship_mappings.keys() { + for (rel_type, _) in config.iter_relationship_mappings() { let table_provider = ctx.table_provider(rel_type) .await @@ -745,12 +745,15 @@ impl CypherQuery { let mut required_tables: HashSet = HashSet::new(); // Use original label/type names (not lowercase keys) for namespace resolution // The namespace needs the original casing to find files on disk - required_tables.extend(config.node_mappings.values().map(|m| m.label.clone())); required_tables.extend( config - .relationship_mappings - .values() - .map(|m| m.relationship_type.clone()), + .iter_node_mappings() + .map(|(_, mapping)| mapping.label.clone()), + ); + required_tables.extend( + config + .iter_relationship_mappings() + .map(|(_, mapping)| mapping.relationship_type.clone()), ); if required_tables.is_empty() { @@ -816,7 +819,7 @@ impl CypherQuery { providers.insert(normalized_table_name.clone(), provider); } - for label in config.node_mappings.keys() { + for (label, _) in config.iter_node_mappings() { let provider = providers .get(label) .ok_or_else(|| GraphError::ConfigError { @@ -831,7 +834,7 @@ impl CypherQuery { catalog = catalog.with_node_source(label, table_source); } - for rel_type in config.relationship_mappings.keys() { + for (rel_type, _) in config.iter_relationship_mappings() { let provider = providers .get(rel_type) .ok_or_else(|| GraphError::ConfigError { From 44f569a3d80020ccd31e0f967404986bc66bd854 Mon Sep 17 00:00:00 2001 From: prrao87 Date: Mon, 26 Oct 2026 09:41:35 -0700 Subject: [PATCH 2/2] test(graph): assert relationship join keys for upper-case types infer_join_keys now finds relationship mappings case-insensitively, so a relationship variable shared by two joined pattern parts adds its endpoint columns as join keys. Previously the exact lookup missed the lower-cased mapping key and returned no keys. --- .../builder/join_builder.rs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/crates/lance-csr/src/datafusion_planner/builder/join_builder.rs b/crates/lance-csr/src/datafusion_planner/builder/join_builder.rs index 97be618..4c74348 100644 --- a/crates/lance-csr/src/datafusion_planner/builder/join_builder.rs +++ b/crates/lance-csr/src/datafusion_planner/builder/join_builder.rs @@ -694,4 +694,47 @@ mod tests { "Shared variables should include 'r'" ); } + + #[test] + fn test_shared_relationship_variable_joins_on_its_endpoints() { + // MATCH (a)-[r:KNOWS]->(b) and (c)-[r:KNOWS]->(d), joined on r. Mapping keys are + // stored lower-case, so the upper-case type must still find the KNOWS mapping. + use crate::datafusion_planner::test_fixtures::{person_knows_config, person_scan}; + let expand = |src: &str, dst: &str| LogicalOperator::Expand { + input: Box::new(person_scan(src)), + source_variable: src.to_string(), + target_variable: dst.to_string(), + target_label: "Person".to_string(), + relationship_types: vec!["KNOWS".to_string()], + direction: crate::ast::RelationshipDirection::Outgoing, + relationship_variable: Some("r".to_string()), + properties: Default::default(), + target_properties: Default::default(), + }; + let join = LogicalOperator::Join { + left: Box::new(expand("a", "b")), + right: Box::new(expand("c", "d")), + join_type: crate::logical_plan::JoinType::Inner, + }; + let planner = DataFusionPlanner::with_catalog(person_knows_config(), make_catalog()); + let analysis = analysis::analyze(&join).unwrap(); + let ctx = crate::datafusion_planner::analysis::PlanningContext::new(&analysis); + let (left, right) = planner.infer_join_keys(&ctx, &join_left(&join), &join_right(&join)); + assert_eq!(left, ["r__src_person_id", "r__dst_person_id"]); + assert_eq!(right, left); + } + + fn join_left(op: &LogicalOperator) -> LogicalOperator { + match op { + LogicalOperator::Join { left, .. } => (**left).clone(), + _ => unreachable!(), + } + } + + fn join_right(op: &LogicalOperator) -> LogicalOperator { + match op { + LogicalOperator::Join { right, .. } => (**right).clone(), + _ => unreachable!(), + } + } }