Skip to content

Commit c7338b2

Browse files
genezhangclaude
andauthored
fix: expression property mappings + CTE node expansion tests (#203)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent fc82925 commit c7338b2

12 files changed

Lines changed: 912 additions & 205 deletions

File tree

src/clickhouse_query_generator/to_sql_query.rs

Lines changed: 57 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1153,16 +1153,38 @@ pub(crate) fn rewrite_expr_for_vlp(
11531153
})
11541154
}
11551155

1156-
RenderExpr::AggregateFnCall(agg) => RenderExpr::AggregateFnCall(AggregateFnCall {
1157-
name: agg.name.clone(),
1158-
args: agg
1159-
.args
1160-
.iter()
1161-
.map(|a| {
1162-
rewrite_expr_for_vlp(a, start_alias, end_alias, path_variable, skip_start_alias)
1163-
})
1164-
.collect(),
1165-
}),
1156+
RenderExpr::AggregateFnCall(agg) => {
1157+
// COUNT(path_variable) → COUNT(*) since each row represents a path
1158+
if let Some(path_var) = path_variable {
1159+
if agg.args.len() == 1 && agg.name.to_lowercase() == "count" {
1160+
if let RenderExpr::TableAlias(alias) = &agg.args[0] {
1161+
if &alias.0 == path_var {
1162+
log::info!("🔧 VLP path aggregate: count({}) → count(*)", path_var);
1163+
return RenderExpr::AggregateFnCall(AggregateFnCall {
1164+
name: agg.name.clone(),
1165+
args: vec![RenderExpr::Star],
1166+
});
1167+
}
1168+
}
1169+
}
1170+
}
1171+
RenderExpr::AggregateFnCall(AggregateFnCall {
1172+
name: agg.name.clone(),
1173+
args: agg
1174+
.args
1175+
.iter()
1176+
.map(|a| {
1177+
rewrite_expr_for_vlp(
1178+
a,
1179+
start_alias,
1180+
end_alias,
1181+
path_variable,
1182+
skip_start_alias,
1183+
)
1184+
})
1185+
.collect(),
1186+
})
1187+
}
11661188

11671189
RenderExpr::ColumnAlias(ColumnAlias(alias_str))
11681190
if path_variable.as_ref() == Some(alias_str) =>
@@ -1563,14 +1585,31 @@ fn rewrite_expr_for_fixed_path(
15631585
})
15641586
}
15651587

1566-
RenderExpr::AggregateFnCall(agg) => RenderExpr::AggregateFnCall(AggregateFnCall {
1567-
name: agg.name.clone(),
1568-
args: agg
1569-
.args
1570-
.iter()
1571-
.map(|a| rewrite_expr_for_fixed_path(a, path_variable, hop_count))
1572-
.collect(),
1573-
}),
1588+
RenderExpr::AggregateFnCall(agg) => {
1589+
// COUNT(path_variable) → COUNT(*) since each row represents a path
1590+
if agg.args.len() == 1 {
1591+
if let RenderExpr::TableAlias(alias) = &agg.args[0] {
1592+
if alias.0 == *path_variable && agg.name.to_lowercase() == "count" {
1593+
log::info!(
1594+
"🔧 Fixed path aggregate: count({}) → count(*)",
1595+
path_variable
1596+
);
1597+
return RenderExpr::AggregateFnCall(AggregateFnCall {
1598+
name: agg.name.clone(),
1599+
args: vec![RenderExpr::Star],
1600+
});
1601+
}
1602+
}
1603+
}
1604+
RenderExpr::AggregateFnCall(AggregateFnCall {
1605+
name: agg.name.clone(),
1606+
args: agg
1607+
.args
1608+
.iter()
1609+
.map(|a| rewrite_expr_for_fixed_path(a, path_variable, hop_count))
1610+
.collect(),
1611+
})
1612+
}
15741613

15751614
// Leave other expressions unchanged
15761615
other => other.clone(),

src/open_cypher_parser/use_clause.rs

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use nom::{
2-
bytes::complete::{tag_no_case, take_while1},
2+
branch::alt,
3+
bytes::complete::{tag, tag_no_case, take_while1},
34
error::context,
5+
sequence::delimited,
46
IResult, Parser,
57
};
68

@@ -10,17 +12,20 @@ use super::{ast::UseClause, common::ws, errors::OpenCypherParsingError};
1012
/// Examples:
1113
/// USE social_network
1214
/// USE ecommerce
13-
/// USE mydb
15+
/// USE `my-database`
1416
pub fn parse_use_clause<'a>(
1517
input: &'a str,
1618
) -> IResult<&'a str, UseClause<'a>, OpenCypherParsingError<'a>> {
1719
let (input, _) = ws(tag_no_case("USE")).parse(input)?;
1820

1921
let (input, database_name) = context(
2022
"Error parsing database name in USE clause",
21-
ws(take_while1(|c: char| {
22-
c.is_alphanumeric() || c == '_' || c == '.'
23-
})),
23+
ws(alt((
24+
// Backtick-quoted identifier: USE `my-database`
25+
delimited(tag("`"), take_while1(|c: char| c != '`'), tag("`")),
26+
// Unquoted identifier
27+
take_while1(|c: char| c.is_alphanumeric() || c == '_' || c == '.'),
28+
))),
2429
)
2530
.parse(input)?;
2631

@@ -92,6 +97,32 @@ mod tests {
9297
assert!(res.is_err(), "Should fail when database name is missing");
9398
}
9499

100+
#[test]
101+
fn test_parse_use_clause_backtick_quoted() {
102+
let input = "USE `my-database` MATCH (n) RETURN n";
103+
let res = parse_use_clause(input);
104+
match res {
105+
Ok((remaining, use_clause)) => {
106+
assert_eq!(use_clause.database_name, "my-database");
107+
assert_eq!(remaining, "MATCH (n) RETURN n");
108+
}
109+
Err(e) => panic!("Failed to parse backtick-quoted USE clause: {:?}", e),
110+
}
111+
}
112+
113+
#[test]
114+
fn test_parse_use_clause_backtick_with_special_chars() {
115+
let input = "USE `zeek_logs` MATCH (n) RETURN n";
116+
let res = parse_use_clause(input);
117+
match res {
118+
Ok((remaining, use_clause)) => {
119+
assert_eq!(use_clause.database_name, "zeek_logs");
120+
assert_eq!(remaining, "MATCH (n) RETURN n");
121+
}
122+
Err(e) => panic!("Failed to parse backtick-quoted USE clause: {:?}", e),
123+
}
124+
}
125+
95126
#[test]
96127
fn test_parse_use_clause_numeric_start() {
97128
let input = "USE 123db";

src/query_planner/logical_plan/match_clause/helpers.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -422,16 +422,15 @@ pub fn register_path_variable(
422422
);
423423

424424
// Then register TableCtx for backward compatibility with code that uses alias_table_ctx_map
425-
plan_ctx.insert_table_ctx(
425+
let mut table_ctx = TableCtx::build(
426426
path_var.to_string(),
427-
TableCtx::build(
428-
path_var.to_string(),
429-
None, // Path variables don't have labels
430-
vec![], // Path variables don't have properties
431-
false, // Not a relationship
432-
true, // Explicitly named by user
433-
),
427+
None, // Path variables don't have labels
428+
vec![], // Path variables don't have properties
429+
false, // Not a relationship
430+
true, // Explicitly named by user
434431
);
432+
table_ctx.set_is_path(true);
433+
plan_ctx.insert_table_ctx(path_var.to_string(), table_ctx);
435434

436435
log::info!(
437436
"📍 Registered path variable '{}' with TypedVariable::Path (start={}, end={}, bounds={:?})",

src/query_planner/optimizer/collect_unwind_elimination.rs

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -518,12 +518,24 @@ impl CollectUnwindElimination {
518518

519519
// Add the source variable as a passthrough item so it's exported
520520
// (the UNWIND alias will be rewritten to reference this)
521-
new_items.push(ProjectionItem {
522-
expression: LogicalExpr::TableAlias(TableAlias(
523-
source.clone(),
524-
)),
525-
col_alias: Some(ColumnAlias(source.clone())),
526-
});
521+
// Only add if source isn't already in the remaining items
522+
let source_already_present =
523+
new_items.iter().any(|item| {
524+
match &item.expression {
525+
LogicalExpr::TableAlias(ta) => {
526+
ta.0 == source
527+
}
528+
_ => false,
529+
}
530+
});
531+
if !source_already_present {
532+
new_items.push(ProjectionItem {
533+
expression: LogicalExpr::TableAlias(
534+
TableAlias(source.clone()),
535+
),
536+
col_alias: Some(ColumnAlias(source.clone())),
537+
});
538+
}
527539

528540
// Update exported aliases: remove collection, add source
529541
let mut new_exported_aliases: Vec<String> = with

src/query_planner/plan_ctx/table_ctx.rs

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ pub struct TableCtx {
3737
/// For relationships: the label of the connected to_node (target)
3838
/// Used to resolve polymorphic relationships
3939
to_node_label: Option<String>,
40+
/// Whether this alias represents a path variable (from `MATCH p = ...`)
41+
is_path: bool,
4042
}
4143

4244
impl TableCtx {
@@ -63,6 +65,7 @@ impl TableCtx {
6365
cte_reference: None,
6466
from_node_label: None,
6567
to_node_label: None,
68+
is_path: false,
6669
}
6770
}
6871

@@ -108,6 +111,7 @@ impl TableCtx {
108111
cte_reference: Some(cte_name),
109112
from_node_label: None,
110113
to_node_label: None,
114+
is_path: false,
111115
}
112116
}
113117

@@ -135,6 +139,9 @@ impl TableCtx {
135139
/// Path variables have no label (None) and are not relationships.
136140
/// Anonymous nodes also have None labels, so we exclude aliases matching "t\d+" pattern.
137141
pub fn is_path_variable(&self) -> bool {
142+
if self.is_path {
143+
return true;
144+
}
138145
if self.is_rel {
139146
return false;
140147
}
@@ -143,9 +150,6 @@ impl TableCtx {
143150
}
144151
// Computed/scalar aliases from WITH (e.g., `WITH size(...) AS cnt`)
145152
// have explicit_alias=true but no labels — they are NOT path variables.
146-
// Note: Real path variables (e.g., `p` in `MATCH p = ...`) are also
147-
// explicit_alias=true, but they are registered via define_path() directly
148-
// in match_clause/traversal.rs, not through this heuristic.
149153
if self.explicit_alias {
150154
return false;
151155
}
@@ -160,6 +164,11 @@ impl TableCtx {
160164
true // No labels, not a relationship, not an anonymous node → path variable
161165
}
162166

167+
/// Mark this TableCtx as a path variable.
168+
pub fn set_is_path(&mut self, val: bool) {
169+
self.is_path = val;
170+
}
171+
163172
// ========================================================================
164173
// Label/Type Access
165174
// ========================================================================

src/render_plan/cte_generation.rs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use std::collections::HashMap;
99
use crate::clickhouse_query_generator::variable_length_cte::WeightCteConfig;
1010
use crate::clickhouse_query_generator::NodeProperty;
1111
use crate::graph_catalog::config::Identifier;
12+
use crate::graph_catalog::expression_parser::PropertyValue;
1213
use crate::graph_catalog::graph_schema::GraphSchema;
1314
use crate::query_planner::logical_expr::LogicalExpr;
1415
use crate::query_planner::logical_plan::LogicalPlan;
@@ -783,6 +784,67 @@ pub fn map_property_to_column_with_relationship_context(
783784
Ok(column.raw().to_string())
784785
}
785786

787+
/// Like `map_property_to_column_with_relationship_context` but preserves
788+
/// the `PropertyValue` variant (Column vs Expression). Use this when the
789+
/// caller needs to generate correct SQL for expression-based mappings.
790+
pub fn map_property_to_property_value(
791+
property: &str,
792+
node_label: &str,
793+
) -> Result<PropertyValue, String> {
794+
let current_schema = crate::server::query_context::get_current_schema_with_fallback();
795+
let schema = current_schema.as_ref().ok_or_else(|| {
796+
format!(
797+
"No schema available for property '{}' on node '{}'",
798+
property, node_label
799+
)
800+
})?;
801+
802+
let node_schema = schema
803+
.all_node_schemas()
804+
.get(node_label)
805+
.ok_or_else(|| format!("Node label '{}' not found in schema", node_label))?;
806+
807+
node_schema
808+
.property_mappings
809+
.get(property)
810+
.cloned()
811+
.ok_or_else(|| {
812+
format!(
813+
"Property '{}' not found for node label '{}'",
814+
property, node_label
815+
)
816+
})
817+
}
818+
819+
/// Like `map_property_to_property_value` but for relationship properties.
820+
pub fn map_rel_property_to_property_value(
821+
property: &str,
822+
relationship_type: &str,
823+
) -> Result<PropertyValue, String> {
824+
let current_schema = crate::server::query_context::get_current_schema_with_fallback();
825+
let schema = current_schema.as_ref().ok_or_else(|| {
826+
format!(
827+
"No schema available for property '{}' on rel '{}'",
828+
property, relationship_type
829+
)
830+
})?;
831+
832+
let rel_schema = schema
833+
.get_rel_schema(relationship_type)
834+
.map_err(|e| format!("Relationship type '{}' not found: {}", relationship_type, e))?;
835+
836+
rel_schema
837+
.property_mappings
838+
.get(property)
839+
.cloned()
840+
.ok_or_else(|| {
841+
format!(
842+
"Property '{}' not found for relationship type '{}'",
843+
property, relationship_type
844+
)
845+
})
846+
}
847+
786848
/// Map a relationship property to its corresponding column name in the schema.
787849
/// This is the relationship equivalent of map_property_to_column_with_schema.
788850
///

0 commit comments

Comments
 (0)