From f71fe3c481b5c8599c24e904d163b8867eae29af Mon Sep 17 00:00:00 2001 From: David Hughes Date: Sun, 25 Oct 2026 09:58:52 -0700 Subject: [PATCH] fix(cypher): preserve integer list precision Keep integer lists as Int64 values and convert numeric lists only at vector boundaries. Add singleton-row planning so standalone RETURN examples execute on configured engines. Issue #3. Implemented with assistance from Codex; coordinated runtime and benchmark validation pending. --- .../src/datafusion_planner/analysis.rs | 2 +- .../src/datafusion_planner/builder/helpers.rs | 4 +- .../src/datafusion_planner/builder/mod.rs | 5 +- .../src/datafusion_planner/vector_ops.rs | 88 ++++++++--------- crates/lance-graph/src/logical_plan.rs | 11 ++- crates/lance-graph/src/parser.rs | 44 +++++++++ docs/cypher.md | 11 ++- docs/vector-search.md | 4 + python/python/tests/test_integer_lists.py | 95 +++++++++++++++++++ 9 files changed, 210 insertions(+), 54 deletions(-) create mode 100644 python/python/tests/test_integer_lists.py diff --git a/crates/lance-graph/src/datafusion_planner/analysis.rs b/crates/lance-graph/src/datafusion_planner/analysis.rs index 89abee0..3329517 100644 --- a/crates/lance-graph/src/datafusion_planner/analysis.rs +++ b/crates/lance-graph/src/datafusion_planner/analysis.rs @@ -256,7 +256,7 @@ fn analyze_operator( } analyze_operator(element, analysis, rel_counter)?; } - LogicalOperator::ComprehensionElement => {} + LogicalOperator::ComprehensionElement | LogicalOperator::SingleRow => {} } Ok(()) } diff --git a/crates/lance-graph/src/datafusion_planner/builder/helpers.rs b/crates/lance-graph/src/datafusion_planner/builder/helpers.rs index f2b8333..34647d5 100644 --- a/crates/lance-graph/src/datafusion_planner/builder/helpers.rs +++ b/crates/lance-graph/src/datafusion_planner/builder/helpers.rs @@ -112,7 +112,7 @@ impl DataFusionPlanner { LogicalOperator::ListComprehension { input, .. } => { Self::collect_variables(input, vars) } - LogicalOperator::ComprehensionElement => {} + LogicalOperator::ComprehensionElement | LogicalOperator::SingleRow => {} } } @@ -176,7 +176,7 @@ impl DataFusionPlanner { LogicalOperator::ShortestPath { input, .. } => Self::collect_labels(input, labels), LogicalOperator::Union { .. } => {} LogicalOperator::ListComprehension { input, .. } => Self::collect_labels(input, labels), - LogicalOperator::ComprehensionElement => {} + LogicalOperator::ComprehensionElement | LogicalOperator::SingleRow => {} } } } diff --git a/crates/lance-graph/src/datafusion_planner/builder/mod.rs b/crates/lance-graph/src/datafusion_planner/builder/mod.rs index cb2d705..c2ca6a4 100644 --- a/crates/lance-graph/src/datafusion_planner/builder/mod.rs +++ b/crates/lance-graph/src/datafusion_planner/builder/mod.rs @@ -22,7 +22,7 @@ mod join_builder; use super::DataFusionPlanner; use crate::error::Result; use crate::logical_plan::*; -use datafusion::logical_expr::LogicalPlan; +use datafusion::logical_expr::{LogicalPlan, LogicalPlanBuilder}; use super::analysis::PlanningContext; @@ -34,6 +34,9 @@ impl DataFusionPlanner { op: &LogicalOperator, ) -> Result { match op { + LogicalOperator::SingleRow => LogicalPlanBuilder::empty(true) + .build() + .map_err(|e| self.plan_error("Failed to build singleton row", e)), LogicalOperator::ScanByLabel { variable, label, diff --git a/crates/lance-graph/src/datafusion_planner/vector_ops.rs b/crates/lance-graph/src/datafusion_planner/vector_ops.rs index 794eec9..a831952 100644 --- a/crates/lance-graph/src/datafusion_planner/vector_ops.rs +++ b/crates/lance-graph/src/datafusion_planner/vector_ops.rs @@ -26,18 +26,7 @@ pub fn extract_vectors(array: &ArrayRef) -> Result>> { }); } let value_array = list_array.value(i); - let float_array = value_array - .as_any() - .downcast_ref::() - .ok_or_else(|| GraphError::ExecutionError { - message: "Expected Float32Array in vector".to_string(), - location: snafu::Location::new(file!(), line!(), column!()), - })?; - - let vec: Vec = (0..float_array.len()) - .map(|j| float_array.value(j)) - .collect(); - vectors.push(vec); + vectors.push(vector_values(value_array.as_ref())?); } return Ok(vectors); } @@ -53,18 +42,7 @@ pub fn extract_vectors(array: &ArrayRef) -> Result>> { }); } let value_array = list_array.value(i); - let float_array = value_array - .as_any() - .downcast_ref::() - .ok_or_else(|| GraphError::ExecutionError { - message: "Expected Float32Array in vector".to_string(), - location: snafu::Location::new(file!(), line!(), column!()), - })?; - - let vec: Vec = (0..float_array.len()) - .map(|j| float_array.value(j)) - .collect(); - vectors.push(vec); + vectors.push(vector_values(value_array.as_ref())?); } return Ok(vectors); } @@ -86,33 +64,32 @@ pub fn extract_single_vector_from_scalar( location: snafu::Location::new(file!(), line!(), column!()), })?; - let list_array = array - .as_any() - .downcast_ref::() + extract_vectors(&array)? + .into_iter() + .next() .ok_or_else(|| GraphError::ExecutionError { - message: "Expected FixedSizeListArray for vector scalar".to_string(), + message: "Empty vector array".to_string(), location: snafu::Location::new(file!(), line!(), column!()), - })?; + }) +} - if list_array.is_empty() { +/// Numeric lists become Float32 only when consumed as vectors. +fn vector_values(values: &dyn Array) -> Result> { + if !values.data_type().is_numeric() || values.null_count() != 0 { return Err(GraphError::ExecutionError { - message: "Empty vector array".to_string(), + message: "Vector elements must be non-null numbers".to_string(), location: snafu::Location::new(file!(), line!(), column!()), }); } - - let value_array = list_array.value(0); - let float_array = value_array - .as_any() - .downcast_ref::() - .ok_or_else(|| GraphError::ExecutionError { - message: "Expected Float32Array in vector".to_string(), - location: snafu::Location::new(file!(), line!(), column!()), + let floats = + arrow::compute::cast(values, &arrow::datatypes::DataType::Float32).map_err(|e| { + GraphError::ExecutionError { + message: format!("Failed to convert vector elements to Float32: {e}"), + location: snafu::Location::new(file!(), line!(), column!()), + } })?; - - Ok((0..float_array.len()) - .map(|j| float_array.value(j)) - .collect()) + let floats = floats.as_any().downcast_ref::().unwrap(); + Ok(floats.values().to_vec()) } /// Compute L2 (Euclidean) distance between two vectors @@ -234,6 +211,31 @@ mod tests { use super::*; use std::sync::Arc; + #[test] + fn test_integer_list_vectors_and_scalar() { + use arrow::array::{Int64Builder, ListBuilder}; + use datafusion::scalar::ScalarValue; + + let mut builder = ListBuilder::new(Int64Builder::new()); + builder.values().append_slice(&[1, 0, 1]); + builder.append(true); + let array: ArrayRef = Arc::new(builder.finish()); + assert_eq!(extract_vectors(&array).unwrap(), vec![vec![1.0, 0.0, 1.0]]); + let scalar = ScalarValue::try_from_array(&array, 0).unwrap(); + assert_eq!( + extract_single_vector_from_scalar(&scalar).unwrap(), + vec![1.0, 0.0, 1.0] + ); + } + + #[test] + fn test_vector_elements_must_be_non_null_numbers() { + use arrow::array::{Int64Array, StringArray}; + + assert!(vector_values(&Int64Array::from(vec![Some(1), None])).is_err()); + assert!(vector_values(&StringArray::from(vec!["1", "2"])).is_err()); + } + #[test] fn test_l2_distance() { let a = vec![1.0, 0.0, 0.0]; diff --git a/crates/lance-graph/src/logical_plan.rs b/crates/lance-graph/src/logical_plan.rs index ed89e08..fd66b89 100644 --- a/crates/lance-graph/src/logical_plan.rs +++ b/crates/lance-graph/src/logical_plan.rs @@ -18,6 +18,8 @@ use std::collections::HashMap; /// A logical plan operator - describes what operation to perform #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum LogicalOperator { + /// One row with no bound variables, for queries without reading clauses. + SingleRow, /// Scan all nodes with a specific label ScanByLabel { variable: String, @@ -385,10 +387,7 @@ impl<'a> LogicalPlanner<'a> { let mut plan = base_plan; if reading_clauses.is_empty() && plan.is_none() { - return Err(GraphError::PlanError { - message: "Query must have at least one MATCH or UNWIND clause".to_string(), - location: snafu::Location::new(file!(), line!(), column!()), - }); + plan = Some(LogicalOperator::SingleRow); } for clause in reading_clauses { @@ -1266,6 +1265,10 @@ impl<'a> LogicalPlanner<'a> { #[allow(clippy::only_used_in_recursion)] fn extract_variable_from_plan(&self, plan: &LogicalOperator) -> Result { match plan { + LogicalOperator::SingleRow => Err(GraphError::PlanError { + message: "a singleton row binds no pattern variable".to_string(), + location: snafu::Location::new(file!(), line!(), column!()), + }), LogicalOperator::ScanByLabel { variable, .. } => Ok(variable.clone()), LogicalOperator::Unwind { alias, .. } => Ok(alias.clone()), LogicalOperator::Expand { diff --git a/crates/lance-graph/src/parser.rs b/crates/lance-graph/src/parser.rs index 9766749..7f215d6 100644 --- a/crates/lance-graph/src/parser.rs +++ b/crates/lance-graph/src/parser.rs @@ -1119,6 +1119,7 @@ fn float32_literal(input: &str) -> IResult<&str, f32> { // Parse vector literal: [0.1, 0.2, 0.3] fn parse_vector_literal(input: &str) -> IResult<&str, ValueExpression> { + let literal = input; let (input, _) = char('[')(input)?; let (input, _) = multispace0(input)?; @@ -1130,6 +1131,17 @@ fn parse_vector_literal(input: &str) -> IResult<&str, ValueExpression> { let (input, _) = multispace0(input)?; let (input, _) = char(']')(input)?; + // Integer lists must retain Int64 precision outside vector operations. + if !literal[..literal.len() - input.len()] + .chars() + .any(|c| matches!(c, '.' | 'e' | 'E')) + { + return Err(nom::Err::Error(nom::error::Error::new( + literal, + nom::error::ErrorKind::Verify, + ))); + } + Ok((input, ValueExpression::VectorLiteral(values))) } @@ -2281,6 +2293,38 @@ mod tests { } } + #[test] + fn test_integer_lists_preserve_precision() { + for literal in ["[1, 2, 3]", "[-19791209300775, 16777217]", "[]"] { + assert!(parse_vector_literal(literal).is_err()); + let (remaining, expression) = value_expression(literal).unwrap(); + assert!(remaining.is_empty()); + let ValueExpression::ScalarFunction { name, args } = expression else { + panic!("expected an ordinary list"); + }; + assert_eq!(name, "__list"); + assert!(args + .iter() + .all(|v| matches!(v, ValueExpression::Literal(PropertyValue::Integer(_))))); + } + let (_, expression) = value_expression("[19791209300775]").unwrap(); + assert_eq!( + expression, + ValueExpression::ScalarFunction { + name: "__list".to_string(), + args: vec![ValueExpression::Literal(PropertyValue::Integer( + 19791209300775 + ))], + } + ); + for literal in ["[1, 2.0]", "[1e-3, 2]", "[-1.0, 2]"] { + assert!(matches!( + value_expression(literal).unwrap().1, + ValueExpression::VectorLiteral(_) + )); + } + } + #[test] fn test_parse_vector_literal() { let result = parse_vector_literal("[0.1, 0.2, 0.3]"); diff --git a/docs/cypher.md b/docs/cypher.md index b7e4674..1339b04 100644 --- a/docs/cypher.md +++ b/docs/cypher.md @@ -177,13 +177,18 @@ read-only subset of openCypher, enough to run all 14 (IC14 without `reduce()`). | `WHERE` | After `MATCH`, `OPTIONAL MATCH` (filters inside the optional part) or `WITH`. | | `WITH [DISTINCT]` | With `ORDER BY`, `LIMIT` and `WHERE`; chain as many as you need. No `SKIP` inside `WITH`. | | `UNWIND list AS x` | | -| `RETURN [DISTINCT]` | With `AS` aliases. | +| `RETURN [DISTINCT]` | With `AS` aliases. Without reading clauses, expressions run on one implicit row. | | `ORDER BY`, `SKIP`, `LIMIT` | `SKIP` and `LIMIT` take integer literals, not parameters. | !!! failure "Not supported" - Writes (`CREATE`, `MERGE`, `SET`, `DELETE`, `REMOVE`), `UNION`, `CALL`, `FOREACH`, and queries with - neither `MATCH` nor `UNWIND`. + Writes (`CREATE`, `MERGE`, `SET`, `DELETE`, `REMOVE`), `UNION`, `CALL`, and `FOREACH`. + +Integer list literals retain signed 64-bit values: `RETURN [1, 2, 3] AS ids` returns integers, and +`UNWIND [1131, 19791209300775] AS id RETURN id` preserves both IDs exactly. Numeric literals with a decimal +point or exponent, such as `[0.1, 1e-3]`, form float32 vectors. Whole-number lists can also be used as query +vectors; conversion to float32 happens inside the vector operation. A configured engine is still required +for standalone `RETURN` queries. ### Patterns diff --git a/docs/vector-search.md b/docs/vector-search.md index 8c5d5d7..21e8a9a 100644 --- a/docs/vector-search.md +++ b/docs/vector-search.md @@ -32,6 +32,10 @@ vector_similarity(a, b, metric) `metric` is a bare keyword (not a string): `cosine`, `l2` or `dot`. One argument must be a property; the other can be a property or a vector literal. +Whole-number query vectors such as `[1, 0]` also work. They remain integer lists until the vector function +converts their elements to float32. Outside vector functions, integer lists preserve signed 64-bit precision. +Vector elements must be non-null numbers. + ```python result = engine.execute(""" MATCH (d:Document) diff --git a/python/python/tests/test_integer_lists.py b/python/python/tests/test_integer_lists.py new file mode 100644 index 0000000..22f9a56 --- /dev/null +++ b/python/python/tests/test_integer_lists.py @@ -0,0 +1,95 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright The lance-csr Authors + +"""Integer list values retain precision; only vector operations convert to float32.""" + +import pyarrow as pa +import pytest +from lance_csr import CypherEngine, GraphConfig + +LARGE_ID = 19791209300775 + + +@pytest.fixture(params=[False, True], ids=["join", "csr"]) +def engine(request): + people = pa.table( + { + "id": [1131, LARGE_ID], + "ids": pa.array([[1131, LARGE_ID], [1, 2]], type=pa.list_(pa.int64())), + "embedding": pa.array( + [[1, 0, 1], [0, 1, 0]], type=pa.list_(pa.float32(), 3) + ), + } + ) + config = GraphConfig.builder().node_table("Person", "id").build() + return CypherEngine(config, {"Person": people}, use_csr=request.param) + + +def test_unwind_large_ids(engine): + result = engine.execute( + f"UNWIND [1131, {LARGE_ID}] AS id " + "MATCH (p:Person {id: id}) RETURN p.id AS id ORDER BY id" + ) + assert result.to_pylist() == [{"id": 1131}, {"id": LARGE_ID}] + assert result.schema.field("id").type == pa.int64() + + +@pytest.mark.parametrize( + ("query", "expected"), + [ + ("RETURN [1, 2, 3] AS l", [1, 2, 3]), + (f"WITH [1131, {LARGE_ID}] AS l RETURN l", [1131, LARGE_ID]), + (f"RETURN [x IN [1131, {LARGE_ID}] | x] AS l", [1131, LARGE_ID]), + (f"RETURN [-{LARGE_ID}, -1, 0] AS l", [-LARGE_ID, -1, 0]), + ], +) +def test_integer_list_values(engine, query, expected): + result = engine.execute(query) + assert result.column(0).to_pylist() == [expected] + assert pa.types.is_int64(result.schema.field(0).type.value_type) + + +def test_integer_list_column_comparison(engine): + result = engine.execute( + f"MATCH (p:Person) WHERE p.ids = [1131, {LARGE_ID}] RETURN p.id AS id" + ) + assert result.to_pylist() == [{"id": 1131}] + + +@pytest.mark.parametrize("metric", ["l2", "cosine", "dot"]) +@pytest.mark.parametrize("function", ["vector_distance", "vector_similarity"]) +def test_integer_query_vector(engine, metric, function): + query = ( + f"MATCH (p:Person) RETURN p.id AS id, " + f"{function}(p.embedding, {{vector}}, {metric}) AS score ORDER BY id" + ) + integer = engine.execute(query.format(vector="[1, 0, 1]")) + decimal = engine.execute(query.format(vector="[1.0, 0.0, 1.0]")) + assert integer.equals(decimal) + + +@pytest.mark.parametrize( + ("query", "expected"), + [ + ("RETURN 1 AS n", [{"n": 1}]), + ("RETURN NULL AS n", [{"n": None}]), + ("RETURN [] AS l", [{"l": []}]), + ("RETURN count(*) AS n", [{"n": 1}]), + ("RETURN 1 AS n LIMIT 0", []), + ("RETURN 1 AS n SKIP 1", []), + ("WITH 2 AS n RETURN n + 1 AS n", [{"n": 3}]), + ("UNWIND [] AS n RETURN n", []), + ], +) +def test_singleton_queries(engine, query, expected): + assert engine.execute(query).to_pylist() == expected + + +def test_singleton_parameters_and_unknown_variables(engine): + assert engine.execute("RETURN $n AS n", {"n": LARGE_ID}).to_pylist() == [ + {"n": LARGE_ID} + ] + with pytest.raises( + ValueError, match="(?i)(unknown|undefined|not defined|not found)" + ): + engine.execute("RETURN missing AS n")