Skip to content

Commit d0840a3

Browse files
genezhangclaude
genezhang
and
claude
authored
fix: centralize denormalized property resolution with from/to awareness (#225)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 0ca5824 commit d0840a3

3 files changed

Lines changed: 171 additions & 26 deletions

File tree

‎src/render_plan/plan_builder_utils.rs‎

Lines changed: 171 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8284,47 +8284,198 @@ pub(crate) fn build_chained_with_match_cte_plan(
82848284
// Performance optimization: Wrap non-ID columns with ANY() when aggregating
82858285
// This allows GROUP BY to only include ID column (more efficient)
82868286

8287-
// Rewrite denormalized node aliases to edge aliases in RenderExpr.
8288-
// For denormalized schemas (e.g., Airport in flights table),
8289-
// PropertyAccessExp(a, OriginCityName) must become
8290-
// PropertyAccessExp(r, OriginCityName) because FROM is `flights AS r`.
8291-
fn rewrite_denormalized_aliases_in_expr(
8287+
// Resolve denormalized property access in RenderExpr.
8288+
// For denormalized schemas (e.g., Airport properties in flights table):
8289+
// 1. Rewrites table alias: a → r (node alias → edge alias)
8290+
// 2. Rewrites property name: uses get_properties_with_table_alias()
8291+
// to map Cypher property → correct DB column (from_node vs to_node aware)
8292+
//
8293+
// This centralizes what was previously split across:
8294+
// - rewrite_expression_with_property_mapping (property names via schema)
8295+
// - separate alias rewriting (table aliases)
8296+
// Both aspects are now handled here using the plan's
8297+
// get_properties_with_table_alias(), which knows the from/to position.
8298+
fn resolve_denormalized_property_in_expr(
82928299
expr: &mut RenderExpr,
82938300
plan: &LogicalPlan,
82948301
) {
82958302
match expr {
82968303
RenderExpr::PropertyAccessExp(prop) => {
8297-
if let Ok((_, Some(edge_alias))) =
8304+
if let Ok((properties, table_alias_override)) =
82988305
plan.get_properties_with_table_alias(&prop.table_alias.0)
82998306
{
8300-
if edge_alias != prop.table_alias.0 {
8301-
log::info!(
8302-
"🔧 Denormalized alias rewrite in WITH: '{}.{}' → '{}.{}'",
8303-
prop.table_alias.0, prop.column.raw(),
8304-
edge_alias, prop.column.raw()
8305-
);
8306-
prop.table_alias =
8307-
crate::render_plan::render_expr::TableAlias(
8308-
edge_alias,
8307+
if let Some(edge_alias) = table_alias_override {
8308+
// This is a denormalized node — resolve both alias and property.
8309+
// The properties list is (cypher_name, db_column) pairs
8310+
// from from_node_properties or to_node_properties,
8311+
// correctly distinguishing Origin* vs Dest* columns.
8312+
let current_col = prop.column.raw().to_string();
8313+
8314+
// Match by Cypher property name first (before schema rewriting),
8315+
// then by DB column name (after schema rewriting).
8316+
// This handles both pre- and post-rewritten expressions.
8317+
let mapped_column = properties
8318+
.iter()
8319+
.find(|(prop_name, _)| *prop_name == current_col)
8320+
.map(|(_, col)| col.clone())
8321+
.or_else(|| {
8322+
// The column may have been rewritten by schema mapping
8323+
// to a DB column (e.g., city → OriginCityName).
8324+
// Check if current_col matches any DB column in our
8325+
// properties list (correct side).
8326+
if properties.iter().any(|(_, col)| *col == current_col) {
8327+
Some(current_col.clone())
8328+
} else {
8329+
// Schema mapped to wrong side's column (e.g., b.city
8330+
// became b.OriginCityName but should be DestCityName).
8331+
// Reverse-lookup: find the Cypher property that maps
8332+
// to current_col using from/to_properties on the
8333+
// node schema, then map through our properties list.
8334+
// Scoped to the alias's node label to avoid false matches.
8335+
use crate::query_planner::logical_expr::expression_rewriter::find_label_for_alias_in_plan;
8336+
use crate::server::query_context::get_current_schema_with_fallback;
8337+
let node_label = find_label_for_alias_in_plan(plan, &prop.table_alias.0);
8338+
if let (Some(label), Some(schema)) = (node_label, get_current_schema_with_fallback()) {
8339+
if let Some(node_schema) = schema.all_node_schemas().get(&label) {
8340+
// Check from_properties
8341+
if let Some(from_props) = &node_schema.from_properties {
8342+
for (cypher_name, db_col) in from_props {
8343+
if *db_col == current_col {
8344+
if let Some((_, correct_col)) = properties
8345+
.iter()
8346+
.find(|(pn, _)| pn == cypher_name)
8347+
{
8348+
log::info!(
8349+
"🔧 Denormalized cross-side fix: '{}.{}' (from '{}') → '{}.{}'",
8350+
prop.table_alias.0, current_col,
8351+
cypher_name, edge_alias, correct_col
8352+
);
8353+
return Some(correct_col.clone());
8354+
}
8355+
}
8356+
}
8357+
}
8358+
// Check to_properties
8359+
if let Some(to_props) = &node_schema.to_properties {
8360+
for (cypher_name, db_col) in to_props {
8361+
if *db_col == current_col {
8362+
if let Some((_, correct_col)) = properties
8363+
.iter()
8364+
.find(|(pn, _)| pn == cypher_name)
8365+
{
8366+
log::info!(
8367+
"🔧 Denormalized cross-side fix: '{}.{}' (from '{}') → '{}.{}'",
8368+
prop.table_alias.0, current_col,
8369+
cypher_name, edge_alias, correct_col
8370+
);
8371+
return Some(correct_col.clone());
8372+
}
8373+
}
8374+
}
8375+
}
8376+
}
8377+
}
8378+
None
8379+
}
8380+
});
8381+
8382+
if let Some(actual_column) = mapped_column {
8383+
if edge_alias != prop.table_alias.0
8384+
|| actual_column != current_col
8385+
{
8386+
log::info!(
8387+
"🔧 Denormalized property resolve in WITH: '{}.{}' → '{}.{}'",
8388+
prop.table_alias.0, current_col,
8389+
edge_alias, actual_column
8390+
);
8391+
prop.table_alias =
8392+
crate::render_plan::render_expr::TableAlias(
8393+
edge_alias,
8394+
);
8395+
prop.column =
8396+
crate::graph_catalog::expression_parser::PropertyValue::Column(
8397+
actual_column,
8398+
);
8399+
}
8400+
} else if edge_alias != prop.table_alias.0 {
8401+
// Property not in any mapping but alias needs rewriting
8402+
log::info!(
8403+
"🔧 Denormalized alias rewrite in WITH: '{}.{}' → '{}.{}'",
8404+
prop.table_alias.0, current_col,
8405+
edge_alias, current_col
83098406
);
8407+
prop.table_alias =
8408+
crate::render_plan::render_expr::TableAlias(
8409+
edge_alias,
8410+
);
8411+
}
83108412
}
83118413
}
83128414
}
83138415
RenderExpr::AggregateFnCall(agg) => {
83148416
for arg in &mut agg.args {
8315-
rewrite_denormalized_aliases_in_expr(arg, plan);
8417+
resolve_denormalized_property_in_expr(arg, plan);
83168418
}
83178419
}
83188420
RenderExpr::ScalarFnCall(f) => {
83198421
for arg in &mut f.args {
8320-
rewrite_denormalized_aliases_in_expr(arg, plan);
8422+
resolve_denormalized_property_in_expr(arg, plan);
83218423
}
83228424
}
83238425
RenderExpr::OperatorApplicationExp(op) => {
83248426
for operand in &mut op.operands {
8325-
rewrite_denormalized_aliases_in_expr(operand, plan);
8427+
resolve_denormalized_property_in_expr(operand, plan);
8428+
}
8429+
}
8430+
RenderExpr::Case(case) => {
8431+
if let Some(expr) = &mut case.expr {
8432+
resolve_denormalized_property_in_expr(expr, plan);
8433+
}
8434+
for (cond, then_expr) in &mut case.when_then {
8435+
resolve_denormalized_property_in_expr(cond, plan);
8436+
resolve_denormalized_property_in_expr(then_expr, plan);
8437+
}
8438+
if let Some(else_expr) = &mut case.else_expr {
8439+
resolve_denormalized_property_in_expr(else_expr, plan);
8440+
}
8441+
}
8442+
RenderExpr::List(items) => {
8443+
for item in items {
8444+
resolve_denormalized_property_in_expr(item, plan);
83268445
}
83278446
}
8447+
RenderExpr::MapLiteral(entries) => {
8448+
for (_, value) in entries {
8449+
resolve_denormalized_property_in_expr(value, plan);
8450+
}
8451+
}
8452+
RenderExpr::ArraySubscript { array, index } => {
8453+
resolve_denormalized_property_in_expr(array, plan);
8454+
resolve_denormalized_property_in_expr(index, plan);
8455+
}
8456+
RenderExpr::ArraySlicing { array, from, to } => {
8457+
resolve_denormalized_property_in_expr(array, plan);
8458+
if let Some(f) = from {
8459+
resolve_denormalized_property_in_expr(f, plan);
8460+
}
8461+
if let Some(t) = to {
8462+
resolve_denormalized_property_in_expr(t, plan);
8463+
}
8464+
}
8465+
RenderExpr::InSubquery(insub) => {
8466+
resolve_denormalized_property_in_expr(&mut insub.expr, plan);
8467+
}
8468+
RenderExpr::ReduceExpr(reduce) => {
8469+
resolve_denormalized_property_in_expr(
8470+
&mut reduce.initial_value,
8471+
plan,
8472+
);
8473+
resolve_denormalized_property_in_expr(&mut reduce.list, plan);
8474+
resolve_denormalized_property_in_expr(
8475+
&mut reduce.expression,
8476+
plan,
8477+
);
8478+
}
83288479
_ => {}
83298480
}
83308481
}
@@ -8484,7 +8635,7 @@ pub(crate) fn build_chained_with_match_cte_plan(
84848635
let expr_result: Result<RenderExpr, _> = expanded_expr.try_into();
84858636
expr_result.ok().map(|mut expr| {
84868637
// Rewrite denormalized node aliases (e.g., a → r)
8487-
rewrite_denormalized_aliases_in_expr(&mut expr, plan_to_render);
8638+
resolve_denormalized_property_in_expr(&mut expr, plan_to_render);
84888639

84898640
// 🔧 FIX: VLP CTE column rewriting for non-TableAlias WITH items
84908641
// When FROM is a VLP/multi-type CTE, PropertyAccess references
@@ -8789,7 +8940,7 @@ pub(crate) fn build_chained_with_match_cte_plan(
87898940
};
87908941
let rewritten = rewrite_expression_with_property_mapping(&item.expression, &rewrite_ctx);
87918942
let expr_vec: Vec<RenderExpr> = rewritten.try_into().ok().map(|mut expr: RenderExpr| {
8792-
rewrite_denormalized_aliases_in_expr(&mut expr, plan_to_render);
8943+
resolve_denormalized_property_in_expr(&mut expr, plan_to_render);
87938944
expr
87948945
}).into_iter().collect();
87958946
expr_vec

‎tests/integration/query_patterns/test_pattern_matrix.py‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1780,7 +1780,6 @@ def test_group_by_2(self):
17801780
result = execute_query(query, "zeek_dns")
17811781
assert "error" not in result, f"Query failed: {result}"
17821782

1783-
@pytest.mark.xfail(reason="No cyclic relationships available")
17841783
def test_with_agg_0(self):
17851784
"""
17861785
WITH clause aggregation
@@ -1790,7 +1789,6 @@ def test_with_agg_0(self):
17901789
result = execute_query(query, "zeek_dns")
17911790
assert "error" not in result, f"Query failed: {result}"
17921791

1793-
@pytest.mark.xfail(reason="No cyclic relationships available")
17941792
def test_with_agg_1(self):
17951793
"""
17961794
WITH clause aggregation
@@ -1800,7 +1798,6 @@ def test_with_agg_1(self):
18001798
result = execute_query(query, "zeek_dns")
18011799
assert "error" not in result, f"Query failed: {result}"
18021800

1803-
@pytest.mark.xfail(reason="No cyclic relationships available")
18041801
def test_with_agg_2(self):
18051802
"""
18061803
WITH clause aggregation

0 commit comments

Comments
 (0)