fix(tck): 402/402 passing — List indexing, type validation, step regex, DELETE detection (#273)

genezhang · claude · web-flow · commit 3837290c33e4 · 2026-04-13T21:37:36.000-07:00
Co-authored-by: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/clickgraph-tck/tests/tck.rs b/clickgraph-tck/tests/tck.rs
@@ -170,11 +170,8 @@ async fn given_having_executed(world: &mut TckWorld, step: &Step) {
 #[given(regex = r"^parameters are:$")]
 async fn given_parameters(world: &mut TckWorld, step: &Step) {
     if let Some(table) = step.table() {
-        // Expect a two-column table: | name | value |
-        // Skip the header row
-        let mut rows = table.rows.iter();
-        rows.next(); // header
-        for row in rows {
+        // Each row IS a key-value pair; there is no header in TCK parameter tables.
+        for row in table.rows.iter() {
             if row.len() >= 2 {
                 let name = row[0].trim().to_string();
                 let value = row[1].trim().to_string();
@@ -366,7 +363,7 @@ async fn then_side_effects(_world: &mut TckWorld) {
     // Accept without assertion.
 }
 
-#[then(regex = r"^a (\w+) should be raised at (?:compile|runtime|any) time: (.+)$")]
+#[then(regex = r"^an? (\w+) should be raised at (?:compile|runtime|any)(?:\s+time)?: (.+)$")]
 async fn then_error_raised(world: &mut TckWorld, _error_type: String, _error_name: String) {
     if world.is_skip() {
         return;
diff --git a/src/clickhouse_query_generator/to_sql_query.rs b/src/clickhouse_query_generator/to_sql_query.rs
@@ -5131,11 +5131,18 @@ impl RenderExpr {
             }
             RenderExpr::ArraySubscript { array, index } => {
                 // Array subscript in ClickHouse: array[index]
-                // Note: Cypher uses 1-based indexing, ClickHouse uses 1-based too
+                // Cypher uses 0-based indexing; ClickHouse uses 1-based.
+                // For integer literals we add +1 at compile time.
+                // For expression indices we emit (expr)+1 without an explicit cast
+                // so ClickHouse's own type checker catches bad types (e.g. floats,
+                // strings) rather than silently coercing them.
+                // Exception: string-literal indices are map-key accesses
+                //   (e.g. top['score']) and must NOT be offset.
                 let array_sql = array.to_sql();
                 let index_sql = match index.as_ref() {
                     RenderExpr::Literal(Literal::Integer(n)) => format!("{}", n + 1),
-                    _ => index.to_sql(),
+                    RenderExpr::Literal(Literal::String(_)) => index.to_sql(),
+                    _ => format!("({})+1", index.to_sql()),
                 };
                 format!("{}[{}]", array_sql, index_sql)
             }
diff --git a/src/query_planner/analyzer/query_validation.rs b/src/query_planner/analyzer/query_validation.rs
@@ -1,3 +1,5 @@
+use std::collections::HashMap;
+use std::fmt;
 use std::sync::Arc;
 
 use crate::{
@@ -7,13 +9,149 @@ use crate::{
             analyzer_pass::{AnalyzerPass, AnalyzerResult},
             errors::{AnalyzerError, Pass},
         },
-        logical_expr::Direction,
-        logical_plan::LogicalPlan,
+        logical_expr::{Direction, Literal, LogicalExpr},
+        logical_plan::{LogicalPlan, ProjectionItem},
         plan_ctx::PlanCtx,
         transformed::Transformed,
     },
 };
 
+// ---------------------------------------------------------------------------
+// Array index type validation helpers
+// ---------------------------------------------------------------------------
+
+/// Non-integer literal kinds that are illegal as array subscript indices.
+#[derive(Debug)]
+enum NonIntegerKind {
+    Boolean,
+    Float,
+    Str,
+    List,
+    Map,
+}
+
+impl fmt::Display for NonIntegerKind {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            NonIntegerKind::Boolean => write!(f, "Boolean"),
+            NonIntegerKind::Float => write!(f, "Float"),
+            NonIntegerKind::Str => write!(f, "String"),
+            NonIntegerKind::List => write!(f, "List"),
+            NonIntegerKind::Map => write!(f, "Map"),
+        }
+    }
+}
+
+/// Returns `Some(kind)` when `expr` is a clearly non-integer literal; `None` otherwise.
+///
+/// Note: `Literal::String` is intentionally excluded — bracket subscripts with string keys
+/// (`map['key']`, `node['prop']`) are valid Cypher map/property access and must not be
+/// rejected. String-keyed array subscripts (invalid) are caught at SQL execution time.
+fn infer_non_integer_kind(expr: &LogicalExpr) -> Option<NonIntegerKind> {
+    match expr {
+        LogicalExpr::Literal(lit) => match lit {
+            Literal::Boolean(_) => Some(NonIntegerKind::Boolean),
+            Literal::Float(_) => Some(NonIntegerKind::Float),
+            Literal::Integer(_) | Literal::String(_) | Literal::Null => None,
+        },
+        LogicalExpr::List(_) => Some(NonIntegerKind::List),
+        LogicalExpr::MapLiteral(_) => Some(NonIntegerKind::Map),
+        _ => None,
+    }
+}
+
+/// Build a map from alias name → non-integer kind using the projection items of a
+/// WITH clause.  Only aliases whose expression is a known non-integer type are included.
+fn build_non_integer_alias_map(items: &[ProjectionItem]) -> HashMap<String, NonIntegerKind> {
+    let mut map = HashMap::new();
+    for item in items {
+        if let Some(col_alias) = &item.col_alias {
+            if let Some(kind) = infer_non_integer_kind(&item.expression) {
+                map.insert(col_alias.0.clone(), kind);
+            }
+        }
+    }
+    map
+}
+
+/// Walk `expr` recursively looking for an `ArraySubscript` whose index is a
+/// known non-integer type (either a direct literal or an alias bound to one).
+/// Returns `Some(error_message)` on the first violation found.
+///
+/// Covers all expression variants that `walk_expression` traverses so that
+/// subscripts nested inside CASE branches, aggregate calls, reduce expressions,
+/// map literals, array slicings, etc. are also checked.
+fn find_invalid_array_subscript(
+    expr: &LogicalExpr,
+    alias_types: &HashMap<String, NonIntegerKind>,
+) -> Option<String> {
+    macro_rules! recurse {
+        ($e:expr) => {
+            find_invalid_array_subscript($e, alias_types)
+        };
+    }
+    match expr {
+        LogicalExpr::ArraySubscript { array, index } => {
+            // Direct non-integer literal index.
+            if let Some(kind) = infer_non_integer_kind(index) {
+                return Some(format!(
+                    "TypeError: invalid array index type {} — must be Integer",
+                    kind
+                ));
+            }
+            // Alias-resolved non-integer index.
+            let alias_name: Option<&str> = match index.as_ref() {
+                LogicalExpr::ColumnAlias(ca) => Some(&ca.0),
+                LogicalExpr::TableAlias(ta) => Some(&ta.0),
+                _ => None,
+            };
+            if let Some(name) = alias_name {
+                if let Some(kind) = alias_types.get(name) {
+                    return Some(format!(
+                        "TypeError: invalid array index type {} (alias '{}') — must be Integer",
+                        kind, name
+                    ));
+                }
+            }
+            recurse!(array).or_else(|| recurse!(index))
+        }
+        LogicalExpr::OperatorApplicationExp(op) | LogicalExpr::Operator(op) => {
+            op.operands.iter().find_map(|o| recurse!(o))
+        }
+        LogicalExpr::ScalarFnCall(f) => f.args.iter().find_map(|a| recurse!(a)),
+        LogicalExpr::AggregateFnCall(f) => f.args.iter().find_map(|a| recurse!(a)),
+        LogicalExpr::List(items) => items.iter().find_map(|i| recurse!(i)),
+        LogicalExpr::MapLiteral(entries) => entries.iter().find_map(|(_, v)| recurse!(v)),
+        LogicalExpr::Case(c) => c
+            .expr
+            .as_ref()
+            .and_then(|e| recurse!(e))
+            .or_else(|| {
+                c.when_then
+                    .iter()
+                    .find_map(|(w, t)| recurse!(w).or_else(|| recurse!(t)))
+            })
+            .or_else(|| c.else_expr.as_ref().and_then(|e| recurse!(e))),
+        LogicalExpr::ReduceExpr(r) => recurse!(&r.initial_value)
+            .or_else(|| recurse!(&r.list))
+            .or_else(|| recurse!(&r.expression)),
+        LogicalExpr::ArraySlicing { array, from, to } => recurse!(array)
+            .or_else(|| from.as_ref().and_then(|f| recurse!(f)))
+            .or_else(|| to.as_ref().and_then(|t| recurse!(t))),
+        _ => None,
+    }
+}
+
+/// Check all expressions in a slice of projection items.
+fn check_items(
+    items: &[ProjectionItem],
+    alias_types: &HashMap<String, NonIntegerKind>,
+) -> Option<String> {
+    items
+        .iter()
+        .find_map(|item| find_invalid_array_subscript(&item.expression, alias_types))
+}
+
 pub struct QueryValidation;
 
 impl AnalyzerPass for QueryValidation {
@@ -25,6 +163,23 @@ impl AnalyzerPass for QueryValidation {
     ) -> AnalyzerResult<Transformed<Arc<LogicalPlan>>> {
         let transformed_plan = match logical_plan.as_ref() {
             LogicalPlan::Projection(projection) => {
+                // Validate array subscript index types.
+                // When the child is a WithClause, build a map of alias → non-integer kind
+                // so we can detect `WITH true AS idx RETURN list[idx]` style errors.
+                let alias_types = if let LogicalPlan::WithClause(wc) = projection.input.as_ref() {
+                    build_non_integer_alias_map(&wc.items)
+                } else {
+                    HashMap::new()
+                };
+
+                for item in &projection.items {
+                    if let Some(err_msg) =
+                        find_invalid_array_subscript(&item.expression, &alias_types)
+                    {
+                        return Err(AnalyzerError::InvalidPlan(err_msg));
+                    }
+                }
+
                 let child_tf = self.analyze_with_graph_schema(
                     projection.input.clone(),
                     plan_ctx,
@@ -232,6 +387,11 @@ impl AnalyzerPass for QueryValidation {
                 graph_joins.rebuild_or_clone(child_tf, logical_plan.clone())
             }
             LogicalPlan::Filter(filter) => {
+                // Validate subscript indices in WHERE predicates.
+                let empty_map = HashMap::new();
+                if let Some(err) = find_invalid_array_subscript(&filter.predicate, &empty_map) {
+                    return Err(AnalyzerError::InvalidPlan(err));
+                }
                 let child_tf =
                     self.analyze_with_graph_schema(filter.input.clone(), plan_ctx, graph_schema)?;
                 filter.rebuild_or_clone(child_tf, logical_plan.clone())
@@ -311,6 +471,27 @@ impl AnalyzerPass for QueryValidation {
                 }
             }
             LogicalPlan::WithClause(with_clause) => {
+                // Validate subscript indices inside WITH projection expressions.
+                let empty_map = HashMap::new();
+                if let Some(err) = check_items(&with_clause.items, &empty_map) {
+                    return Err(AnalyzerError::InvalidPlan(err));
+                }
+                // Build alias→type map so WHERE/ORDER BY can resolve alias references.
+                let alias_types = build_non_integer_alias_map(&with_clause.items);
+                if let Some(ref wc) = with_clause.where_clause {
+                    if let Some(err) = find_invalid_array_subscript(wc, &alias_types) {
+                        return Err(AnalyzerError::InvalidPlan(err));
+                    }
+                }
+                if let Some(ref order_by) = with_clause.order_by {
+                    for item in order_by {
+                        if let Some(err) =
+                            find_invalid_array_subscript(&item.expression, &alias_types)
+                        {
+                            return Err(AnalyzerError::InvalidPlan(err));
+                        }
+                    }
+                }
                 let child_tf = self.analyze_with_graph_schema(
                     with_clause.input.clone(),
                     plan_ctx,
