From 5c7773d9c958fe535991d1b26d73e5f64599c6e3 Mon Sep 17 00:00:00 2001 From: David Hughes Date: Wed, 28 Oct 2026 19:33:35 -0700 Subject: [PATCH 1/3] fix(query): resolve anonymous relationship patterns across labels Resolve omitted relationship types from configured endpoints and retain typed boundary-node identities for whole-node returns. --- .../src/datafusion_planner/analysis.rs | 2 +- .../src/datafusion_planner/builder/helpers.rs | 4 +- .../src/datafusion_planner/builder/mod.rs | 6 +- .../datafusion_planner/builder/path_ops.rs | 2 + crates/lance-csr/src/logical_plan.rs | 44 +++++++-- crates/lance-csr/src/path_match.rs | 25 +++++ crates/lance-csr/src/pattern_alternatives.rs | 37 +++++++- crates/lance-csr/tests/test_engine.rs | 95 +++++++++++++++++++ docs/cypher.md | 6 ++ .../python/tests/test_engine_alternation.py | 12 +++ 10 files changed, 221 insertions(+), 12 deletions(-) diff --git a/crates/lance-csr/src/datafusion_planner/analysis.rs b/crates/lance-csr/src/datafusion_planner/analysis.rs index 31fa380..613b250 100644 --- a/crates/lance-csr/src/datafusion_planner/analysis.rs +++ b/crates/lance-csr/src/datafusion_planner/analysis.rs @@ -271,7 +271,7 @@ fn analyze_operator( analysis.required_datasets.insert(label.clone()); } } - LogicalOperator::PathMatch { input, path } => { + LogicalOperator::PathMatch { input, path, .. } => { analyze_operator(input, analysis, rel_counter)?; for node in std::iter::once(&path.start_node).chain(path.segments.iter().map(|s| &s.end_node)) diff --git a/crates/lance-csr/src/datafusion_planner/builder/helpers.rs b/crates/lance-csr/src/datafusion_planner/builder/helpers.rs index b418918..ffe66c0 100644 --- a/crates/lance-csr/src/datafusion_planner/builder/helpers.rs +++ b/crates/lance-csr/src/datafusion_planner/builder/helpers.rs @@ -96,7 +96,7 @@ impl DataFusionPlanner { LogicalOperator::OptionalMatch { optional, .. } => { Self::collect_variables(optional, vars); } - LogicalOperator::PathMatch { input, path } => { + LogicalOperator::PathMatch { input, path, .. } => { Self::collect_variables(input, vars); vars.extend( std::iter::once(&path.start_node) @@ -191,7 +191,7 @@ impl DataFusionPlanner { LogicalOperator::OptionalMatch { optional, .. } => { Self::collect_labels(optional, labels) } - LogicalOperator::PathMatch { input, path } => { + LogicalOperator::PathMatch { input, path, .. } => { Self::collect_labels(input, labels); for node in std::iter::once(&path.start_node) .chain(path.segments.iter().map(|s| &s.end_node)) diff --git a/crates/lance-csr/src/datafusion_planner/builder/mod.rs b/crates/lance-csr/src/datafusion_planner/builder/mod.rs index e07b4d1..e55db0c 100644 --- a/crates/lance-csr/src/datafusion_planner/builder/mod.rs +++ b/crates/lance-csr/src/datafusion_planner/builder/mod.rs @@ -164,7 +164,11 @@ impl DataFusionPlanner { optional, keys, } => self.build_optional_match(ctx, input, optional, keys), - LogicalOperator::PathMatch { input, path } => self.build_path_match(ctx, input, path), + LogicalOperator::PathMatch { + input, + path, + node_values, + } => self.build_path_match(ctx, input, path, *node_values), LogicalOperator::ShortestPath { input, source_variable, diff --git a/crates/lance-csr/src/datafusion_planner/builder/path_ops.rs b/crates/lance-csr/src/datafusion_planner/builder/path_ops.rs index 7cf18c2..a541159 100644 --- a/crates/lance-csr/src/datafusion_planner/builder/path_ops.rs +++ b/crates/lance-csr/src/datafusion_planner/builder/path_ops.rs @@ -17,6 +17,7 @@ impl DataFusionPlanner { ctx: &mut PlanningContext, input: &LogicalOperator, path: &PathPattern, + node_values: bool, ) -> Result { let error = |message: String| GraphError::UnsupportedFeature { feature: message, @@ -198,6 +199,7 @@ impl DataFusionPlanner { plans, PathSpec { variable, + node_values, sources: ctx.path_sources.clone(), nodes, edges, diff --git a/crates/lance-csr/src/logical_plan.rs b/crates/lance-csr/src/logical_plan.rs index 21c1f24..606899a 100644 --- a/crates/lance-csr/src/logical_plan.rs +++ b/crates/lance-csr/src/logical_plan.rs @@ -147,6 +147,8 @@ pub enum LogicalOperator { PathMatch { input: Box, path: PathPattern, + /// Preserve typed node identities for untyped relationship queries. + node_values: bool, }, /// `shortestPath` / `allShortestPaths` between two variables bound in `input`; adds @@ -267,6 +269,8 @@ pub struct LogicalPlanner<'a> { /// The types each relationship variable was first written with. relationship_types: HashMap>, rich_paths: bool, + /// Untyped MATCH results need per-row identities across endpoint label alternatives. + wildcard_relationships: bool, /// The rows a projection or WHERE started from, while its EXISTS marks and /// comprehensions are being added: each is seeded from these, not from the rows with /// the previous ones joined on, so planned copies grow linearly, not exponentially. @@ -287,6 +291,7 @@ impl<'a> LogicalPlanner<'a> { path_labels: HashMap::new(), relationship_types: HashMap::new(), rich_paths: false, + wildcard_relationships: false, seed_source: None, } } @@ -441,6 +446,14 @@ impl<'a> LogicalPlanner<'a> { /// Convert a Cypher AST to a logical plan pub fn plan(&mut self, query: &CypherQuery) -> Result { + self.wildcard_relationships = false; + query.for_each_query(&mut |scope| { + self.wildcard_relationships |= scope.all_reading_clauses().any(|clause| { + matches!(clause, ReadingClause::Match(m) if m.patterns.iter().any(|pattern| { + matches!(pattern, GraphPattern::Path(path) if path.segments.iter().any(|segment| segment.relationship.types.is_empty())) + })) + }); + }); self.rich_paths = crate::path_value::needs_values(query, self.config); self.plan_query(query, None, false) } @@ -544,6 +557,7 @@ impl<'a> LogicalPlanner<'a> { .with_adjacency(self.adjacency); planner.calls = self.calls; planner.rich_paths = self.rich_paths; + planner.wildcard_relationships = self.wildcard_relationships; if preserve_entities { planner.variables = imports.0.clone(); planner.node_lists = imports.1.clone(); @@ -637,6 +651,7 @@ impl<'a> LogicalPlanner<'a> { .with_adjacency(self.adjacency); inner.calls = self.calls; inner.rich_paths = self.rich_paths; + inner.wildcard_relationships = self.wildcard_relationships; for name in &imports { if let Some(label) = self.variables.get(name) { inner.variables.insert(name.clone(), label.clone()); @@ -881,7 +896,11 @@ impl<'a> LogicalPlanner<'a> { }; let match_clause = &inferred; let retain_variable = self.retain_variable_alternation(&match_clause.patterns); - if crate::pattern_alternatives::needs_expansion(&match_clause.patterns, retain_variable) { + if crate::pattern_alternatives::needs_expansion( + &match_clause.patterns, + retain_variable, + self.config, + ) { let branches = crate::pattern_alternatives::expand( &match_clause.patterns, self.config, @@ -945,11 +964,12 @@ impl<'a> LogicalPlanner<'a> { plan = Some(self.plan_shortest_path(plan, path)?) } GraphPattern::Path(path) - if self.rich_paths - && path - .segments - .iter() - .any(|s| s.relationship.variable.is_some()) => + if self.wildcard_relationships + || (self.rich_paths + && path + .segments + .iter() + .any(|s| s.relationship.variable.is_some())) => { let mut path = path.clone(); path.variable = Some(format!("_path_value_{}", self.variables.len())); @@ -1659,6 +1679,7 @@ impl<'a> LogicalPlanner<'a> { return Ok(LogicalOperator::PathMatch { input: Box::new(plan.expect("both ends planned")), path: path.clone(), + node_values: self.wildcard_relationships, }); } Ok(LogicalOperator::ShortestPath { @@ -2026,6 +2047,7 @@ impl<'a> LogicalPlanner<'a> { Ok(LogicalOperator::PathMatch { input: Box::new(base), path, + node_values: self.wildcard_relationships, }) } @@ -2117,6 +2139,16 @@ impl<'a> LogicalPlanner<'a> { match &item.expression { ValueExpression::Variable(var) => { match self.variables.get(var) { + Some(label) + if self.wildcard_relationships + && label != "Unwound" + && label != "Path" => + { + projections.push(ProjectionItem { + expression: item.expression.clone(), + alias: alias.clone(), + }); + } // if it is a node variable, expand to all properties Some(label) if label != "Unwound" && label != "Path" => { let mapping = self.config.get_node_mapping(label).ok_or_else(|| { diff --git a/crates/lance-csr/src/path_match.rs b/crates/lance-csr/src/path_match.rs index 07895b1..68077f7 100644 --- a/crates/lance-csr/src/path_match.rs +++ b/crates/lance-csr/src/path_match.rs @@ -54,6 +54,8 @@ pub(crate) struct Step { #[derive(Debug)] pub(crate) struct PathSpec { pub variable: String, + /// Expose typed boundary-node values alongside their property columns. + pub node_values: bool, pub sources: crate::path_inventory::SourceBatches, pub nodes: Vec, pub edges: Vec, @@ -73,6 +75,7 @@ pub(crate) struct PathMatchNode { #[derive(Debug, Clone)] enum Output { + NodeValue { boundary: usize }, RelationshipValue { step: usize }, Node { boundary: usize, property: usize }, Relationship { step: usize, property: usize }, @@ -97,6 +100,20 @@ impl PathMatchNode { let mut fields = Vec::new(); let mut outputs = Vec::new(); let input = inputs[0].schema(); + for (boundary, binding) in spec + .bindings + .iter() + .enumerate() + .filter(|_| spec.node_values) + { + let name = binding.variable.to_lowercase(); + if !input.has_column_with_unqualified_name(&name) + && !fields.iter().any(|f: &Field| f.name() == &name) + { + fields.push(Field::new(name, node_type.clone(), true)); + outputs.push(Output::NodeValue { boundary }); + } + } for (boundary, binding) in spec.bindings.iter().enumerate().skip(1) { if input.has_column_with_unqualified_name(&binding.id_column) { continue; @@ -409,6 +426,13 @@ impl ExecutionPlan for PathMatchExec { .collect::>()?; for output in &outputs { let (array, positions) = match output { + Output::NodeValue { boundary } => { + let positions = UInt32Array::from_iter_values( + paths.iter().map(|p| p.nodes[p.boundaries[*boundary]]), + ); + columns.push(take(&p.graph.inventory.nodes, &positions, None)?); + continue; + } Output::RelationshipValue { step } => { let positions = UInt32Array::from_iter_values( paths.iter().map(|p| p.edges[p.boundaries[*step]]), @@ -892,6 +916,7 @@ mod tests { }; let spec = PathSpec { variable: "p".into(), + node_values: false, sources: Default::default(), nodes: vec![NodeSource { label: "Person".into(), diff --git a/crates/lance-csr/src/pattern_alternatives.rs b/crates/lance-csr/src/pattern_alternatives.rs index 4af644a..85572e2 100644 --- a/crates/lance-csr/src/pattern_alternatives.rs +++ b/crates/lance-csr/src/pattern_alternatives.rs @@ -48,6 +48,21 @@ pub fn infer_labels( bound: &HashMap, ) -> Vec { let mut patterns = patterns.to_vec(); + // An omitted relationship type ranges over the configured graph, not a fictitious Node table. + let mut types: Vec<_> = config + .iter_relationship_mappings() + .map(|(_, mapping)| mapping.relationship_type.clone()) + .collect(); + types.sort(); + for pattern in &mut patterns { + if let GraphPattern::Path(path) = pattern { + for segment in &mut path.segments { + if segment.relationship.types.is_empty() { + segment.relationship.types = types.clone(); + } + } + } + } let mut known: HashMap = bound .iter() .filter(|(_, l)| config.get_node_mapping(l).is_some()) @@ -154,13 +169,31 @@ pub fn infer_labels( patterns } -/// True when any relationship has several types or any node several labels. -pub fn needs_expansion(patterns: &[GraphPattern], retain_variable: bool) -> bool { +/// Branch on alternatives and fixed-hop unlabeled endpoints that mappings can resolve. +pub fn needs_expansion( + patterns: &[GraphPattern], + retain_variable: bool, + config: &GraphConfig, +) -> bool { let nodes_multi = |n: &NodePattern| n.labels.len() > 1; patterns.iter().any(|p| match p { GraphPattern::Node(n) => nodes_multi(n), GraphPattern::Path(path) => { + let unresolved = path.start_node.labels.is_empty() + || path.segments.iter().any(|s| s.end_node.labels.is_empty()); + let fixed_endpoints = path.segments.iter().all(|s| { + s.relationship.length.is_none() + && !s.relationship.types.is_empty() + && s.relationship.types.iter().all(|kind| { + config + .get_relationship_mapping(kind) + .is_some_and(|mapping| { + mapping.source_label.is_some() && mapping.target_label.is_some() + }) + }) + }); nodes_multi(&path.start_node) + || (unresolved && fixed_endpoints) || path.segments.iter().any(|s| { (s.relationship.types.len() > 1 && !(retain_variable && s.relationship.length.is_some())) diff --git a/crates/lance-csr/tests/test_engine.rs b/crates/lance-csr/tests/test_engine.rs index 5e5adcc..b4c54d8 100644 --- a/crates/lance-csr/tests/test_engine.rs +++ b/crates/lance-csr/tests/test_engine.rs @@ -451,3 +451,98 @@ async fn path_exists_matches_the_join_plan() { assert!(!plan.contains("PathExistsExec"), "{query}\n{plan}"); } } + +#[tokio::test] +async fn anonymous_patterns_keep_mixed_label_identity_and_query_semantics() { + use arrow_array::{cast::AsArray, StructArray}; + for use_csr in [false, true] { + let project = RecordBatch::try_from_iter([ + ( + "id", + Arc::new(Int64Array::from(vec![1])) as arrow_array::ArrayRef, + ), + ( + "title", + Arc::new(StringArray::from(vec!["Project one"])) as arrow_array::ArrayRef, + ), + ]) + .unwrap(); + let cfg = GraphConfig::builder() + .node_table("Person", "id") + .node_table("Project", "id") + .rel_table("KNOWS", "Person", "Person", "src_id", "dst_id") + .rel_table("WORKS_ON", "Person", "Project", "src_id", "dst_id") + .build() + .unwrap(); + let engine = CypherEngine::new( + cfg, + HashMap::from([ + ("Person".into(), EngineSource::Arrow(people())), + ("Project".into(), EngineSource::Arrow(project)), + ( + "KNOWS".into(), + EngineSource::Arrow(knows(&[(1, 2), (1, 2), (1, 1)])), + ), + ("WORKS_ON".into(), EngineSource::Arrow(knows(&[(1, 1)]))), + ]), + use_csr, + ) + .await + .unwrap(); + for query in [ + "MATCH (a)-[]-(b) RETURN a,b", + "MATCH (a)-[r]-(b) RETURN a,r,b", + "MATCH p = (a)-[]-(b) RETURN p", + ] { + let batch = engine.execute(query, HashMap::new()).await.unwrap(); + assert_eq!(batch.num_rows(), 7, "{query}"); + if let Some(column) = batch.column_by_name("a") { + let entity = column.as_any().downcast_ref::().unwrap(); + let identity = entity + .column_by_name("__lance_csr_identity") + .unwrap() + .as_struct(); + let labels = identity.column_by_name("label").unwrap().as_string::(); + assert!((0..labels.len()).any(|row| labels.value(row) == "Person")); + assert!((0..labels.len()).any(|row| labels.value(row) == "Project")); + } + } + for (query, expected) in [ + ("MATCH (a)-[]-(b) RETURN count(*) AS total", 7), + ( + "MATCH (a)-[]-(b) WHERE a.name = 'Alice' RETURN count(*) AS total", + 4, + ), + ] { + let batch = engine.execute(query, HashMap::new()).await.unwrap(); + assert_eq!( + batch + .column_by_name("total") + .unwrap() + .as_primitive::() + .value(0), + expected + ); + } + let alias = engine + .execute( + "MATCH (a)-[]-(b) WITH a AS friend RETURN friend", + HashMap::new(), + ) + .await + .unwrap(); + assert_eq!(alias.num_rows(), 7); + assert!(matches!( + alias.schema().field(0).data_type(), + DataType::Struct(_) + )); + let limit = engine + .execute( + "MATCH p = (a)-[]-(b) RETURN p SKIP 2 LIMIT 3", + HashMap::new(), + ) + .await + .unwrap(); + assert_eq!(limit.num_rows(), 3); + } +} diff --git a/docs/cypher.md b/docs/cypher.md index 4ce29eb..ec3e291 100644 --- a/docs/cypher.md +++ b/docs/cypher.md @@ -305,6 +305,12 @@ MATCH p = shortestPath((a:Person {id: 1})-[:KNOWS*]-(b:Person {id: 4})) RETURN length(p), nodes(p) ``` +- **Untyped relationships** (`MATCH (a)-[]-(b) RETURN a,b` or + `MATCH path = (a)-[]-(b) RETURN path`) range over the configured relationship types. + Endpoint mappings resolve unlabeled nodes without combining identities from different tables. + Undirected matches include both orientations; a self-loop appears once and parallel edges remain + distinct. Whole-node returns in these queries use typed Arrow entity records. The existing + alternative-branch bound still applies. - **Variable length** defaults to a minimum of 1 and a maximum of 20 hops; a maximum above 20 is rejected. `*0..` also matches the source itself, once per input row; `*0..0` matches only that identity. The start and end must have the same label and key for a zero-hop match. Eligible CSR paths use one iterative diff --git a/python/python/tests/test_engine_alternation.py b/python/python/tests/test_engine_alternation.py index e85af9c..853df52 100644 --- a/python/python/tests/test_engine_alternation.py +++ b/python/python/tests/test_engine_alternation.py @@ -162,3 +162,15 @@ def test_collect_distinct(datasets): (1, ["a"], 3), (2, ["b"], 1), ] + + +@pytest.mark.parametrize("use_csr", [False, True]) +def test_anonymous_relationships_return_all_types(datasets, use_csr): + engine = CypherEngine(config(), datasets, use_csr=use_csr) + result = engine.execute("MATCH (a)-[]-(b) RETURN a,b") + assert result.num_rows == 12 + labels = {row["a"]["__lance_csr_identity"]["label"] for row in result.to_pylist()} + assert labels == {"Person", "Post", "Comment"} + paths = engine.execute("MATCH path = (a)-[]-(b) RETURN path") + assert paths.num_rows == 12 + assert all(row["path"]["length"] == 1 for row in paths.to_pylist()) From e48e8e68ef41aefc419bc6c5f0e83f474bfd6655 Mon Sep 17 00:00:00 2001 From: David Hughes Date: Mon, 2 Nov 2026 16:59:02 -0800 Subject: [PATCH 2/3] fix(query): support bound string predicate operands --- .../src/datafusion_planner/expression.rs | 266 ++++++------------ .../datafusion_planner/scalar_functions.rs | 61 +++- crates/lance-csr/src/parser.rs | 57 +++- crates/lance-csr/src/semantic.rs | 35 ++- crates/lance-csr/tests/test_engine.rs | 137 +++++++++ docs/cypher.md | 15 + python/python/tests/test_string_parameters.py | 51 ++++ 7 files changed, 416 insertions(+), 206 deletions(-) create mode 100644 python/python/tests/test_string_parameters.py diff --git a/crates/lance-csr/src/datafusion_planner/expression.rs b/crates/lance-csr/src/datafusion_planner/expression.rs index 4617199..0764852 100644 --- a/crates/lance-csr/src/datafusion_planner/expression.rs +++ b/crates/lance-csr/src/datafusion_planner/expression.rs @@ -154,21 +154,18 @@ pub(crate) fn to_df_boolean_expr(expr: &BooleanExpression) -> Expr { BE::Contains { expression, substring, - } => { - // CONTAINS is equivalent to LIKE '%substring%' - let pattern = format!("%{}%", substring); - create_like_expr(expression, &pattern, false) - } - BE::StartsWith { expression, prefix } => { - // STARTS WITH is equivalent to LIKE 'prefix%' - let pattern = format!("{}%", prefix); - create_like_expr(expression, &pattern, false) - } - BE::EndsWith { expression, suffix } => { - // ENDS WITH is equivalent to LIKE '%suffix' - let pattern = format!("%{}", suffix); - create_like_expr(expression, &pattern, false) - } + } => super::scalar_functions::call( + "__string_contains", + vec![to_df_value_expr(expression), lit(substring.clone())], + ), + BE::StartsWith { expression, prefix } => super::scalar_functions::call( + "__string_starts_with", + vec![to_df_value_expr(expression), lit(prefix.clone())], + ), + BE::EndsWith { expression, suffix } => super::scalar_functions::call( + "__string_ends_with", + vec![to_df_value_expr(expression), lit(suffix.clone())], + ), } } @@ -278,10 +275,30 @@ pub(crate) fn to_df_value_expr(expr: &ValueExpression) -> Expr { "abs" | "round" | "floor" | "ceil" | "sqrt" => { super::math_functions::call(name, args.iter().map(to_df_value_expr).collect()) } - "left" | "right" | "substring" | "replace" | "split" | "trim" | "ltrim" - | "rtrim" | "size" | "reverse" | "last" | "tail" | "keys" | "__entity_keys" - | "labels" | "startnode" | "endnode" | "__predicate_all" | "__predicate_any" - | "__predicate_none" | "__predicate_single" => { + "left" + | "right" + | "substring" + | "replace" + | "split" + | "trim" + | "ltrim" + | "rtrim" + | "size" + | "reverse" + | "last" + | "tail" + | "keys" + | "__entity_keys" + | "labels" + | "startnode" + | "endnode" + | "__predicate_all" + | "__predicate_any" + | "__predicate_none" + | "__predicate_single" + | "__string_contains" + | "__string_starts_with" + | "__string_ends_with" => { super::scalar_functions::call(name, args.iter().map(to_df_value_expr).collect()) } "range" => { @@ -869,7 +886,10 @@ mod tests { if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { assert!(!like_expr.negated, "Should not be negated"); assert!(!like_expr.case_insensitive, "Should be case sensitive"); - assert_eq!(like_expr.escape_char, None, "Should have no escape char"); + assert_eq!( + like_expr.escape_char, None, + "LIKE keeps its explicit pattern semantics" + ); match *like_expr.expr { Expr::Column(ref col_expr) => { assert_eq!(col_expr.name(), "p__name"); @@ -902,7 +922,10 @@ mod tests { like_expr.case_insensitive, "ILIKE should be case insensitive" ); - assert_eq!(like_expr.escape_char, None, "Should have no escape char"); + assert_eq!( + like_expr.escape_char, None, + "LIKE keeps its explicit pattern semantics" + ); match *like_expr.expr { Expr::Column(ref col_expr) => { assert_eq!(col_expr.name(), "p__name"); @@ -985,168 +1008,43 @@ mod tests { } #[test] - fn test_boolean_expr_contains() { - let expr = BooleanExpression::Contains { - expression: ValueExpression::Property(PropertyRef { - variable: "p".into(), - property: "name".into(), - }), - substring: "ali".into(), - }; - - if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { - assert!(!like_expr.negated, "Should not be negated"); - assert!(!like_expr.case_insensitive, "Should be case sensitive"); - assert_eq!(like_expr.escape_char, None, "Should have no escape char"); - - // Check the expression is the column - match *like_expr.expr { - Expr::Column(ref col_expr) => { - assert_eq!(col_expr.name(), "p__name"); - } - other => panic!("Expected column expression, got {:?}", other), - } - - // Check pattern is '%ali%' - match *like_expr.pattern { - Expr::Literal(ref scalar, _) => { - let s = format!("{:?}", scalar); - assert!(s.contains("%ali%"), "Pattern should be '%ali%', got: {}", s); - } - other => panic!("Expected literal pattern, got {:?}", other), - } - } else { - panic!("Expected Like expression"); - } - } - - #[test] - fn test_boolean_expr_starts_with() { - let expr = BooleanExpression::StartsWith { - expression: ValueExpression::Property(PropertyRef { - variable: "p".into(), - property: "email".into(), - }), - prefix: "admin".into(), - }; - - if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { - assert!(!like_expr.negated, "Should not be negated"); - assert!(!like_expr.case_insensitive, "Should be case sensitive"); - - // Check the expression is the column - match *like_expr.expr { - Expr::Column(ref col_expr) => { - assert_eq!(col_expr.name(), "p__email"); - } - other => panic!("Expected column expression, got {:?}", other), - } - - // Check pattern is 'admin%' - match *like_expr.pattern { - Expr::Literal(ref scalar, _) => { - let s = format!("{:?}", scalar); - assert!( - s.contains("admin%"), - "Pattern should be 'admin%', got: {}", - s - ); - } - other => panic!("Expected literal pattern, got {:?}", other), - } - } else { - panic!("Expected Like expression"); - } - } - - #[test] - fn test_boolean_expr_ends_with() { - let expr = BooleanExpression::EndsWith { - expression: ValueExpression::Property(PropertyRef { - variable: "p".into(), - property: "email".into(), - }), - suffix: "@example.com".into(), - }; - - if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { - assert!(!like_expr.negated, "Should not be negated"); - assert!(!like_expr.case_insensitive, "Should be case sensitive"); - - // Check the expression is the column - match *like_expr.expr { - Expr::Column(ref col_expr) => { - assert_eq!(col_expr.name(), "p__email"); - } - other => panic!("Expected column expression, got {:?}", other), - } - - // Check pattern is '%@example.com' - match *like_expr.pattern { - Expr::Literal(ref scalar, _) => { - let s = format!("{:?}", scalar); - assert!( - s.contains("%@example.com"), - "Pattern should be '%@example.com', got: {}", - s - ); - } - other => panic!("Expected literal pattern, got {:?}", other), - } - } else { - panic!("Expected Like expression"); - } - } - - #[test] - fn test_boolean_expr_contains_case_sensitivity() { - // Test that CONTAINS is case-sensitive (case_insensitive = false) - let expr = BooleanExpression::Contains { - expression: ValueExpression::Property(PropertyRef { - variable: "p".into(), - property: "name".into(), - }), - substring: "Test".into(), - }; - - if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { - assert!( - !like_expr.case_insensitive, - "CONTAINS should be case-sensitive by default" - ); - } else { - panic!("Expected Like expression"); - } - } - - #[test] - fn test_boolean_expr_string_operators_with_variable() { - // Test that string operators work with variable references, not just properties - let expr = BooleanExpression::Contains { - expression: ValueExpression::Variable("name".into()), - substring: "test".into(), - }; - - if let Expr::Like(like_expr) = to_df_boolean_expr(&expr) { - match *like_expr.expr { - Expr::Column(ref col_expr) => { - assert_eq!( - col_expr.name(), - "name", - "Should reference variable directly" - ); - } - other => panic!("Expected column expression, got {:?}", other), - } - } else { - panic!("Expected Like expression"); + fn string_predicates_keep_literal_operands_and_column_identity() { + let left = ValueExpression::Property(PropertyRef { + variable: "p".into(), + property: "name".into(), + }); + for (predicate, name) in [ + ( + BooleanExpression::Contains { + expression: left.clone(), + substring: "%_".into(), + }, + "__string_contains", + ), + ( + BooleanExpression::StartsWith { + expression: left.clone(), + prefix: "%_".into(), + }, + "__string_starts_with", + ), + ( + BooleanExpression::EndsWith { + expression: left, + suffix: "%_".into(), + }, + "__string_ends_with", + ), + ] { + let Expr::ScalarFunction(function) = to_df_boolean_expr(&predicate) else { + panic!("Expected literal string predicate") + }; + assert_eq!(function.name(), name); + assert_eq!(function.args[0], col("p__name")); + assert_eq!(function.args[1], lit("%_")); } } - // ======================================================================== - // Unit tests for to_df_value_expr() - // ======================================================================== - #[test] fn test_value_expr_property() { let expr = ValueExpression::Property(PropertyRef { @@ -1459,7 +1357,7 @@ mod tests { } #[test] - fn test_tolower_with_contains_produces_valid_like() { + fn test_tolower_with_contains_preserves_literal_text() { // This is the bug scenario: toLower(s.name) CONTAINS 'offer' // Previously returned lit(0) which caused type coercion error let tolower_expr = ValueExpression::ScalarFunction { @@ -1478,10 +1376,10 @@ mod tests { let df_expr = to_df_boolean_expr(&contains_expr); let s = format!("{:?}", df_expr); - // Should be a Like expression with lower() on the column, not lit(0) + // The predicate must retain lower() and the literal, rather than a placeholder. assert!( - s.contains("Like"), - "Should be a LIKE expression, got: {}", + s.contains("__string_contains"), + "Should be a literal string predicate, got: {}", s ); assert!( diff --git a/crates/lance-csr/src/datafusion_planner/scalar_functions.rs b/crates/lance-csr/src/datafusion_planner/scalar_functions.rs index d877529..8f8bdbc 100644 --- a/crates/lance-csr/src/datafusion_planner/scalar_functions.rs +++ b/crates/lance-csr/src/datafusion_planner/scalar_functions.rs @@ -7,7 +7,8 @@ use std::sync::Arc; use arrow::array::{ - new_null_array, Array, AsArray, Int64Array, ListBuilder, StringArray, StringBuilder, + new_null_array, Array, AsArray, BooleanArray, Int64Array, ListBuilder, StringArray, + StringBuilder, }; use arrow::compute::{cast, cast_with_options, CastOptions}; use arrow_schema::{DataType, Field}; @@ -47,6 +48,7 @@ impl ScalarUDFImpl for CypherScalar { fn return_type(&self, types: &[DataType]) -> Result { for (i, t) in types.iter().enumerate() { let valid = match self.name.as_str() { + "__string_contains" | "__string_starts_with" | "__string_ends_with" => true, "range" => t.is_integer(), "__predicate_all" | "__predicate_any" | "__predicate_none" | "__predicate_single" => list_field(t) @@ -70,9 +72,13 @@ impl ScalarUDFImpl for CypherScalar { DataType::Struct(fields) => fields[2].data_type().clone(), _ => DataType::Null, }, - "__predicate_all" | "__predicate_any" | "__predicate_none" | "__predicate_single" => { - DataType::Boolean - } + "__predicate_all" + | "__predicate_any" + | "__predicate_none" + | "__predicate_single" + | "__string_contains" + | "__string_starts_with" + | "__string_ends_with" => DataType::Boolean, "size" => DataType::Int64, "range" => DataType::List(Arc::new(Field::new("item", DataType::Int64, true))), "split" | "keys" | "__entity_keys" | "labels" => { @@ -92,6 +98,19 @@ impl ScalarUDFImpl for CypherScalar { fn invoke_with_args(&self, mut args: ScalarFunctionArgs) -> Result { let output_type = args.return_type().clone(); + if matches!( + self.name.as_str(), + "__string_contains" | "__string_starts_with" | "__string_ends_with" + ) && args + .args + .iter() + .any(|a| !is_string(&a.data_type()) && !a.data_type().is_null()) + { + return Ok(ColumnarValue::Array(new_null_array( + &DataType::Boolean, + args.number_rows, + ))); + } if args.args[0].data_type().is_null() && self.name != "range" { return Ok(ColumnarValue::Array(new_null_array( &output_type, @@ -263,6 +282,25 @@ impl ScalarUDFImpl for CypherScalar { }) .collect::, _>>()?; let strings = arrays[0].as_string::(); + if matches!( + self.name.as_str(), + "__string_contains" | "__string_starts_with" | "__string_ends_with" + ) { + let patterns = arrays[1].as_string::(); + let result: BooleanArray = strings + .iter() + .zip(patterns.iter()) + .map(|(text, pattern)| { + text.zip(pattern) + .map(|(text, pattern)| match self.name.as_str() { + "__string_contains" => text.contains(pattern), + "__string_starts_with" => text.starts_with(pattern), + _ => text.ends_with(pattern), + }) + }) + .collect(); + return Ok(ColumnarValue::Array(Arc::new(result))); + } if self.name == "size" { return Ok(ColumnarValue::Array(Arc::new( strings @@ -379,6 +417,21 @@ pub(crate) fn sql_expr( } let args = &function.args; Ok(match function.name() { + "__string_contains" | "__string_starts_with" | "__string_ends_with" => { + if args.iter().any(|a| { + a.get_type(schema) + .is_ok_and(|t| !is_string(&t) && !t.is_null()) + }) { + Some(datafusion::logical_expr::lit(ScalarValue::Boolean(None))) + } else { + let function = match function.name() { + "__string_contains" => datafusion::functions::string::contains(), + "__string_starts_with" => datafusion::functions::string::starts_with(), + _ => datafusion::functions::string::ends_with(), + }; + Some(function.call(args.clone())) + } + } "size" if list_field(&args[0].get_type(schema)?).is_some() => Some(cast( length::array_length_udf().call(args.clone()), DataType::Int64, diff --git a/crates/lance-csr/src/parser.rs b/crates/lance-csr/src/parser.rs index 18c7a48..53f45d1 100644 --- a/crates/lance-csr/src/parser.rs +++ b/crates/lance-csr/src/parser.rs @@ -567,6 +567,27 @@ fn exists_subquery(input: &str) -> IResult<&str, BooleanExpression> { )) } +// Keep literal AST constructors compatible while expression operands use normal parameter traversal. +fn string_match( + left: ValueExpression, + right: ValueExpression, + function: &str, + literal: impl FnOnce(ValueExpression, String) -> BooleanExpression, +) -> BooleanExpression { + if let ValueExpression::Literal(PropertyValue::String(text)) = right { + literal(left, text) + } else { + BooleanExpression::Comparison { + left: ValueExpression::ScalarFunction { + name: function.into(), + args: vec![left, right], + }, + operator: ComparisonOperator::Equal, + right: ValueExpression::Literal(PropertyValue::Boolean(true)), + } + } +} + fn comparison_expression(input: &str) -> IResult<&str, BooleanExpression> { let (input, _) = multispace0(input)?; let (input, left) = value_expression(input)?; @@ -621,14 +642,16 @@ fn comparison_expression(input: &str) -> IResult<&str, BooleanExpression> { } // Match CONTAINS substring if let Ok((input_after_contains, (_, _, substring))) = - tuple((tag_no_case("CONTAINS"), multispace0, string_literal))(input) + tuple((tag_no_case("CONTAINS"), multispace0, value_expression))(input) { return Ok(( input_after_contains, - BooleanExpression::Contains { - expression: left, - substring, - }, + string_match(left, substring, "__string_contains", |expression, text| { + BooleanExpression::Contains { + expression, + substring: text, + } + }), )); } // Match STARTS WITH prefix (note: multi-word operator) @@ -637,15 +660,17 @@ fn comparison_expression(input: &str) -> IResult<&str, BooleanExpression> { multispace1, tag_no_case("WITH"), multispace0, - string_literal, + value_expression, ))(input) { return Ok(( input_after_starts, - BooleanExpression::StartsWith { - expression: left, - prefix, - }, + string_match(left, prefix, "__string_starts_with", |expression, text| { + BooleanExpression::StartsWith { + expression, + prefix: text, + } + }), )); } // Match ENDS WITH suffix (note: multi-word operator) @@ -654,15 +679,17 @@ fn comparison_expression(input: &str) -> IResult<&str, BooleanExpression> { multispace1, tag_no_case("WITH"), multispace0, - string_literal, + value_expression, ))(input) { return Ok(( input_after_ends, - BooleanExpression::EndsWith { - expression: left, - suffix, - }, + string_match(left, suffix, "__string_ends_with", |expression, text| { + BooleanExpression::EndsWith { + expression, + suffix: text, + } + }), )); } // Match is null diff --git a/crates/lance-csr/src/semantic.rs b/crates/lance-csr/src/semantic.rs index f9909a0..3d63f13 100644 --- a/crates/lance-csr/src/semantic.rs +++ b/crates/lance-csr/src/semantic.rs @@ -616,10 +616,28 @@ impl SemanticAnalyzer { }); } } - "date_part" | "to_epoch_ms" | "length" | "nodes" | "size" | "left" - | "right" | "split" | "replace" | "__index" => { + "date_part" + | "to_epoch_ms" + | "length" + | "nodes" + | "size" + | "left" + | "right" + | "split" + | "replace" + | "__index" + | "__string_contains" + | "__string_starts_with" + | "__string_ends_with" => { let want = match function_name.as_str() { - "date_part" | "__index" | "left" | "right" | "split" => 2, + "date_part" + | "__index" + | "left" + | "right" + | "split" + | "__string_contains" + | "__string_starts_with" + | "__string_ends_with" => 2, "replace" => 3, _ => 1, }; @@ -1168,6 +1186,17 @@ impl SemanticAnalyzer { // Property is valid if present in at least one label's property_fields // Use case-insensitive comparison let prop_lower = prop.to_lowercase(); + if var_info + .labels + .iter() + .filter_map(|label| self.config.get_node_mapping(label)) + .any(|mapping| { + mapping.id_field.eq_ignore_ascii_case(prop) + || prop.eq_ignore_ascii_case("id") + }) + { + continue 'prop; + } for fields in &label_property_sets { if fields.iter().any(|f| f.to_lowercase() == prop_lower) { continue 'prop; diff --git a/crates/lance-csr/tests/test_engine.rs b/crates/lance-csr/tests/test_engine.rs index b4c54d8..bcadda3 100644 --- a/crates/lance-csr/tests/test_engine.rs +++ b/crates/lance-csr/tests/test_engine.rs @@ -546,3 +546,140 @@ async fn anonymous_patterns_keep_mixed_label_identity_and_query_semantics() { assert_eq!(limit.num_rows(), 3); } } + +#[tokio::test] +async fn string_predicate_parameters_preserve_literals_nulls_and_cached_values() { + use serde_json::json; + let batch = RecordBatch::try_from_iter(vec![ + ( + "id", + Arc::new(Int64Array::from(vec![1, 2, 3, 4, 5])) as arrow_array::ArrayRef, + ), + ( + "name", + Arc::new(StringArray::from(vec![ + Some("Ada_%\\Zoë"), + Some("Ada"), + Some("ada"), + None, + Some(""), + ])) as arrow_array::ArrayRef, + ), + ]) + .unwrap(); + let engine = CypherEngine::new( + config(), + HashMap::from([ + ("Person".into(), EngineSource::Arrow(batch)), + ("KNOWS".into(), EngineSource::Arrow(knows(&[]))), + ]), + false, + ) + .await + .unwrap(); + for (operator, needle, expected) in [ + ("CONTAINS", json!("Ada"), 2), + ("CONTAINS", json!("ada"), 1), + ("CONTAINS", json!("%"), 1), + ("CONTAINS", json!("_"), 1), + ("CONTAINS", json!("\\"), 1), + ("CONTAINS", json!("Zoë"), 1), + ("CONTAINS", json!(""), 4), + ("CONTAINS", json!(null), 0), + ("CONTAINS", json!(42), 0), + ("CONTAINS", json!("' OR true //"), 0), + ("STARTS WITH", json!("Ada"), 2), + ("STARTS WITH", json!(""), 4), + ("STARTS WITH", json!(null), 0), + ("ENDS WITH", json!("Zoë"), 1), + ("ENDS WITH", json!(""), 4), + ("ENDS WITH", json!(null), 0), + ] { + let text = format!( + "MATCH (p:Person) WHERE p.name {operator} $needle RETURN p.name AS name ORDER BY p.id" + ); + let batches: Vec<_> = engine + .execute_stream(&text, HashMap::from([("needle".into(), needle.clone())])) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!( + batches.iter().map(RecordBatch::num_rows).sum::(), + expected, + "{operator} {needle}" + ); + } + // Literal metacharacters are text, as for bound operands, rather than SQL LIKE wildcards. + for (pattern, expected) in [("%", 1), ("_", 1), ("", 4)] { + let text = + format!("MATCH (p:Person) WHERE p.name CONTAINS '{pattern}' RETURN p.name AS name"); + let batches: Vec<_> = engine + .execute_stream(&text, HashMap::new()) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!( + batches.iter().map(RecordBatch::num_rows).sum::(), + expected + ); + } + let text = "MATCH (p:Person) WITH p WHERE p.name STARTS WITH left($needle, $n) RETURN p.name AS name ORDER BY p.id SKIP $skip LIMIT $take"; + let batches: Vec<_> = engine + .execute_stream( + text, + HashMap::from([ + ("needle".into(), json!("Ada")), + ("n".into(), json!(1)), + ("skip".into(), json!(1)), + ("take".into(), json!(1)), + ]), + ) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!(batches.iter().flat_map(names).collect::>(), ["Ada"]); + assert!(engine + .execute_stream( + "MATCH (p:Person) WHERE p.name CONTAINS $missing RETURN p", + HashMap::new() + ) + .await + .is_err()); +} + +#[tokio::test] +async fn parameterized_key_map_with_declared_property_projection() { + let config = GraphConfig::builder() + .with_node_mapping( + lance_csr::NodeMapping::new("Person", "id").with_properties(vec!["name".into()]), + ) + .build() + .unwrap(); + let engine = CypherEngine::new( + config, + HashMap::from([("Person".into(), EngineSource::Arrow(people()))]), + false, + ) + .await + .unwrap(); + let batches: Vec<_> = engine + .execute_stream( + "MATCH (p:Person {id:$id}) RETURN p.name AS name", + HashMap::from([("id".into(), serde_json::json!(1))]), + ) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!( + batches.iter().flat_map(names).collect::>(), + ["Alice"] + ); +} diff --git a/docs/cypher.md b/docs/cypher.md index ec3e291..5cbf02b 100644 --- a/docs/cypher.md +++ b/docs/cypher.md @@ -97,6 +97,21 @@ engine.execute( ).to_pylist() # [{'name': 'Alice'}, {'name': 'Carol'}] ``` +`CONTAINS`, `STARTS WITH` and `ENDS WITH` accept bound strings and expressions: + +```python +engine.execute( + "MATCH (p:Person) WHERE p.name STARTS WITH $prefix RETURN p.name AS name", + {"prefix": "Ali"}, +).to_pylist() # [{'name': 'Alice'}] +``` + +Text matching is case-sensitive. Use `toLower()` on both operands for case-insensitive matching. +Percent signs, underscores and backslashes are literal characters. An empty search string matches +all non-null strings; a null or non-string operand produces null and does not pass a `WHERE` filter. +Use `($name IS NULL OR p.name CONTAINS $name)` for an optional text filter. Parameters bind values, +not raw property names or labels: `p.name = $name` is valid; `p.$property` is not. + Pagination also accepts parameters, for example `RETURN 1 AS n LIMIT $take` with `{"take": 1}`. `WITH ... LIMIT $take` uses the same rules. Missing, null, boolean, fractional, negative and overflowing counts raise `ValueError`; arbitrary expressions and `WITH SKIP` remain unsupported. diff --git a/python/python/tests/test_string_parameters.py b/python/python/tests/test_string_parameters.py new file mode 100644 index 0000000..4dd88d5 --- /dev/null +++ b/python/python/tests/test_string_parameters.py @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright The lance-csr Authors + +"""String operands use bound expressions with literal text semantics.""" + +import pyarrow as pa +import pytest +from lance_csr import CypherEngine, GraphConfig + + +@pytest.mark.parametrize("csr", [False, True], ids=["join", "csr"]) +def test_bound_string_operands(csr: bool) -> None: + config = GraphConfig.builder().node_table("Person", "id").build() + engine = CypherEngine( + config, + { + "Person": pa.table( + {"id": [1, 2, 3, 4], "name": ["Ada_%\\Zoë", "Ada", None, ""]} + ) + }, + use_csr=csr, + ) + for operator, needle, ids in [ + ("CONTAINS", "Ada", [1, 2]), + ("CONTAINS", "%", [1]), + ("CONTAINS", "_", [1]), + ("CONTAINS", "\\", [1]), + ("CONTAINS", "Zoë", [1]), + ("CONTAINS", "", [1, 2, 4]), + ("CONTAINS", None, []), + ("CONTAINS", 42, []), + ("STARTS WITH", "Ada", [1, 2]), + ("ENDS WITH", "Zoë", [1]), + ("STARTS WITH", "", [1, 2, 4]), + ("ENDS WITH", None, []), + ]: + query = ( + f"MATCH (p:Person) WHERE p.name {operator} $text " + "RETURN p.id AS id ORDER BY id" + ) + assert engine.execute(query, {"text": needle}).to_pylist() == [ + {"id": value} for value in ids + ] + assert engine.execute( + "MATCH (p:Person) WHERE toLower(p.name) STARTS WITH toLower($text) " + "RETURN p.id AS id ORDER BY id SKIP $skip LIMIT $take", + {"text": "ADA", "skip": 1, "take": 1}, + ).to_pylist() == [{"id": 2}] + assert engine.execute( + "MATCH (p:Person {id: $id}) RETURN p.id AS id", {"id": 2} + ).to_pylist() == [{"id": 2}] From 2a7c38dce5911096bd7b0ba285301d3c5c001274 Mon Sep 17 00:00:00 2001 From: prrao87 Date: Tue, 3 Nov 2026 20:18:46 -0500 Subject: [PATCH 3/3] fix(planner): keep reused relationship variables typed in untyped-pattern detection A reused relationship variable such as `(c)-[r]->(d)` takes its type from where `r` was first written with one (#83). Detecting untyped patterns from the raw AST counted it as untyped and sent the query down the typed-node-value path, which failed with a duplicate `r` field. --- crates/lance-csr/src/logical_plan.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/crates/lance-csr/src/logical_plan.rs b/crates/lance-csr/src/logical_plan.rs index 606899a..2988c17 100644 --- a/crates/lance-csr/src/logical_plan.rs +++ b/crates/lance-csr/src/logical_plan.rs @@ -13,7 +13,7 @@ use crate::error::{GraphError, Result}; use crate::{ast::*, GraphConfig}; use serde::{Deserialize, Serialize}; use snafu::Location; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; /// A logical plan operator - describes what operation to perform #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -446,14 +446,28 @@ impl<'a> LogicalPlanner<'a> { /// Convert a Cypher AST to a logical plan pub fn plan(&mut self, query: &CypherQuery) -> Result { - self.wildcard_relationships = false; + let mut relationships = Vec::new(); query.for_each_query(&mut |scope| { - self.wildcard_relationships |= scope.all_reading_clauses().any(|clause| { - matches!(clause, ReadingClause::Match(m) if m.patterns.iter().any(|pattern| { - matches!(pattern, GraphPattern::Path(path) if path.segments.iter().any(|segment| segment.relationship.types.is_empty())) - })) - }); + for clause in scope.all_reading_clauses() { + if let ReadingClause::Match(m) = clause { + for pattern in &m.patterns { + if let GraphPattern::Path(path) = pattern { + relationships + .extend(path.segments.iter().map(|s| s.relationship.clone())); + } + } + } + } }); + // A reused relationship variable takes its types from where it was written with them. + let typed: HashSet<_> = relationships + .iter() + .filter(|r| !r.types.is_empty()) + .filter_map(|r| r.variable.as_ref()) + .collect(); + self.wildcard_relationships = relationships + .iter() + .any(|r| r.types.is_empty() && r.variable.as_ref().is_none_or(|v| !typed.contains(v))); self.rich_paths = crate::path_value::needs_values(query, self.config); self.plan_query(query, None, false) }