Skip to content

Commit cad745f

Browse files
committed
fix: Address PR #64 review comments
- WebSocket Close frame returns EOF for graceful disconnect (websocket.rs) - Downgrade info/warn logs to trace/debug in hot paths (result_transformer.rs, select_builder.rs) - Fix AUTHORED from_id: user_id -> author_id (social_benchmark.yaml) - Fix LIKED like_date mapping (social_benchmark.yaml) - Update tests for new untyped pattern behavior
1 parent 85c8984 commit cad745f

6 files changed

Lines changed: 52 additions & 42 deletions

File tree

benchmarks/social_network/schemas/social_benchmark.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ graph_schema:
6262
- type: AUTHORED
6363
database: brahmand
6464
table: posts_bench
65-
from_id: user_id
65+
from_id: author_id
6666
to_id: post_id
6767
edge_id: post_id
6868
from_node: User
@@ -78,7 +78,7 @@ graph_schema:
7878
from_node: User
7979
to_node: Post
8080
property_mappings:
81-
like_date: liked_at
81+
like_date: like_date
8282

8383
- type: FRIENDS_WITH
8484
database: brahmand

src/query_planner/analyzer/match_type_inference.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -566,7 +566,7 @@ mod tests {
566566
let schema = create_test_schema_with_relationships();
567567
let plan_ctx = PlanCtx::new(Arc::new(schema.clone()));
568568

569-
// Both nodes untyped with multiple relationships - cannot infer
569+
// Both nodes untyped with multiple relationships - returns all rel types for UNION expansion
570570
let result = infer_relationship_type_from_nodes(
571571
&None,
572572
&None,
@@ -576,6 +576,10 @@ mod tests {
576576
)
577577
.unwrap();
578578

579-
assert_eq!(result, None);
579+
// Now returns all relationship types for UNION expansion (changed behavior)
580+
assert!(result.is_some());
581+
let rel_types = result.unwrap();
582+
assert!(rel_types.contains(&"FOLLOWS".to_string()));
583+
assert!(rel_types.contains(&"LIKES".to_string()));
580584
}
581585
}

src/query_planner/logical_plan/match_clause/tests.rs

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -204,22 +204,21 @@ fn test_traverse_node_pattern_existing_node() {
204204
}
205205

206206
#[test]
207-
fn test_traverse_node_pattern_empty_node_error() {
208-
let mut plan_ctx = PlanCtx::new_empty();
207+
fn test_traverse_node_pattern_unnamed_node_gets_alias() {
208+
// Previously this would error, but now we auto-generate aliases for unnamed nodes
209+
let graph_schema = create_test_schema_with_relationships();
210+
let mut plan_ctx = PlanCtx::new(Arc::new(graph_schema));
209211
let initial_plan = Arc::new(LogicalPlan::Empty);
210212

211213
let node_pattern = ast::NodePattern {
212-
name: None, // Empty node
213-
labels: Some(vec!["Person"]),
214+
name: None, // Unnamed node - should get auto-generated alias
215+
labels: Some(vec!["User"]), // Use a label that exists in test schema
214216
properties: None,
215217
};
216218

217219
let result = traverse_node_pattern(&node_pattern, initial_plan, &mut plan_ctx);
218-
assert!(result.is_err());
219-
match result.unwrap_err() {
220-
LogicalPlanError::EmptyNode => (), // Expected error
221-
_ => panic!("Expected EmptyNode error"),
222-
}
220+
// Should succeed now - auto-generates alias like "t1"
221+
assert!(result.is_ok());
223222
}
224223

225224
#[test]
@@ -1180,7 +1179,7 @@ fn test_infer_relationship_type_no_matches() {
11801179

11811180
#[test]
11821181
fn test_infer_relationship_type_both_untyped_multi_schema() {
1183-
// ()-[r]->() with multiple relationships should return None
1182+
// ()-[r]->() with multiple relationships returns all types for UNION expansion
11841183
let schema = create_test_schema_with_relationships();
11851184
let plan_ctx = PlanCtx::new(Arc::new(schema.clone()));
11861185

@@ -1193,8 +1192,11 @@ fn test_infer_relationship_type_both_untyped_multi_schema() {
11931192
)
11941193
.expect("Should not error");
11951194

1196-
// Both nodes untyped and schema has 3 relationships - cannot infer
1197-
assert!(result.is_none());
1195+
// Now returns all relationship types for UNION expansion (changed behavior)
1196+
assert!(result.is_some());
1197+
let rel_types = result.unwrap();
1198+
assert!(rel_types.contains(&"FOLLOWS".to_string()));
1199+
assert!(rel_types.contains(&"LIKES".to_string()));
11981200
}
11991201

12001202
#[test]

src/render_plan/select_builder.rs

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ impl SelectBuilder for LogicalPlan {
4040
&self,
4141
plan_ctx: Option<&crate::query_planner::plan_ctx::PlanCtx>,
4242
) -> Result<Vec<SelectItem>, RenderBuildError> {
43-
log::warn!("🔍🔍🔍 extract_select_items CALLED on plan type");
43+
log::trace!("🔍🔍🔍 extract_select_items CALLED on plan type");
4444
crate::debug_println!("DEBUG: extract_select_items called on: {:?}", self);
4545
let select_items = match &self {
4646
LogicalPlan::Empty => vec![],
@@ -81,7 +81,7 @@ impl SelectBuilder for LogicalPlan {
8181
}
8282
}
8383
LogicalPlan::GraphRel(graph_rel) => {
84-
log::warn!(
84+
log::trace!(
8585
"🔍 GraphRel.extract_select_items: alias={}, path_variable={:?}",
8686
graph_rel.alias,
8787
graph_rel.path_variable
@@ -166,10 +166,13 @@ impl SelectBuilder for LogicalPlan {
166166

167167
// NEW APPROACH: Use TypedVariable for type/source checking
168168
if let Some(plan_ctx) = plan_ctx {
169-
log::warn!(" 🔍 Looking up '{}' in plan_ctx...", table_alias.0);
169+
log::trace!(" 🔍 Looking up '{}' in plan_ctx...", table_alias.0);
170170
match plan_ctx.lookup_variable(&table_alias.0) {
171171
Some(typed_var) if typed_var.is_entity() => {
172-
log::warn!(" ✓ Found ENTITY variable '{}'", table_alias.0);
172+
log::trace!(
173+
" ✓ Found ENTITY variable '{}'",
174+
table_alias.0
175+
);
173176
// Entity (Node or Relationship) - expand properties
174177
match &typed_var.source() {
175178
VariableSource::Match => {
@@ -192,7 +195,7 @@ impl SelectBuilder for LogicalPlan {
192195
);
193196
}
194197
_ => {
195-
log::warn!("⚠️ Entity variable '{}' has unexpected source, treating as scalar", table_alias.0);
198+
log::debug!("⚠️ Entity variable '{}' has unexpected source, treating as scalar", table_alias.0);
196199
select_items.push(SelectItem {
197200
expression: RenderExpr::ColumnAlias(
198201
ColumnAlias(table_alias.0.clone()),
@@ -206,7 +209,10 @@ impl SelectBuilder for LogicalPlan {
206209
}
207210
}
208211
Some(typed_var) if typed_var.is_scalar() => {
209-
log::warn!(" ✓ Found SCALAR variable '{}'", table_alias.0);
212+
log::trace!(
213+
" ✓ Found SCALAR variable '{}'",
214+
table_alias.0
215+
);
210216
// Scalar - single item, no expansion
211217
match &typed_var.source() {
212218
VariableSource::Cte { cte_name } => {
@@ -245,7 +251,7 @@ impl SelectBuilder for LogicalPlan {
245251
);
246252
}
247253
_ => {
248-
log::warn!(" ✗ Variable '{}' NOT FOUND or not a recognized type in plan_ctx", table_alias.0);
254+
log::trace!(" ✗ Variable '{}' NOT FOUND or not a recognized type in plan_ctx", table_alias.0);
249255
// Unknown variable - check if it's a path by looking for GraphRel
250256
if let Some(graph_rel) =
251257
self.find_graph_rel_for_path(&table_alias.0)
@@ -280,7 +286,7 @@ impl SelectBuilder for LogicalPlan {
280286
);
281287
} else {
282288
// Really unknown - fallback to old logic
283-
log::warn!("⚠️ Variable '{}' not found in TypedVariable registry or GraphRel, using fallback logic", table_alias.0);
289+
log::debug!("⚠️ Variable '{}' not found in TypedVariable registry or GraphRel, using fallback logic", table_alias.0);
284290
self.fallback_table_alias_expansion(
285291
table_alias,
286292
item,
@@ -408,7 +414,7 @@ impl SelectBuilder for LogicalPlan {
408414
);
409415

410416
if cte_ref.columns.is_empty() {
411-
log::warn!("⚠️ CteEntityRef '{}' has no columns - falling back to TableAlias", cte_ref.alias);
417+
log::debug!("⚠️ CteEntityRef '{}' has no columns - falling back to TableAlias", cte_ref.alias);
412418
select_items.push(SelectItem {
413419
expression: RenderExpr::TableAlias(RenderTableAlias(
414420
cte_ref.alias.clone(),
@@ -586,7 +592,7 @@ impl SelectBuilder for LogicalPlan {
586592
LogicalPlan::Unwind(u) => u.input.extract_select_items(plan_ctx)?,
587593
LogicalPlan::CartesianProduct(cp) => {
588594
// Combine select items from both sides
589-
log::warn!("🔍 CartesianProduct.extract_select_items START");
595+
log::trace!("🔍 CartesianProduct.extract_select_items START");
590596
let left_items = cp.left.extract_select_items(plan_ctx)?;
591597
log::warn!(
592598
"🔍 CartesianProduct.extract_select_items: left side returned {} items",
@@ -609,7 +615,7 @@ impl SelectBuilder for LogicalPlan {
609615
graph_node.input.extract_select_items(plan_ctx)?
610616
}
611617
LogicalPlan::WithClause(wc) => {
612-
log::warn!("🔍 WithClause.extract_select_items: calling extract on input");
618+
log::trace!("🔍 WithClause.extract_select_items: calling extract on input");
613619
let items = wc.input.extract_select_items(plan_ctx)?;
614620
log::warn!(
615621
"🔍 WithClause.extract_select_items DONE: extracted {} items from input plan",
@@ -711,7 +717,7 @@ impl LogicalPlan {
711717
);
712718
}
713719
_ => {
714-
log::warn!("⚠️ No properties found for base table entity '{}'", alias);
720+
log::debug!("⚠️ No properties found for base table entity '{}'", alias);
715721
}
716722
}
717723
}
@@ -733,7 +739,7 @@ impl LogicalPlan {
733739

734740
// Parse CTE name to get aliases and compute FROM alias
735741
let from_alias = self.compute_from_alias_from_cte_name(cte_name);
736-
log::info!("🔍 CTE '{}' → FROM alias '{}'", cte_name, from_alias);
742+
log::trace!("🔍 CTE '{}' → FROM alias '{}'", cte_name, from_alias);
737743

738744
// Get labels from TypedVariable
739745
let labels = match typed_var {
@@ -1002,7 +1008,7 @@ impl LogicalPlan {
10021008
);
10031009
vs.is_denormalized
10041010
} else {
1005-
log::info!("🔍 Relationship '{}' center is NOT ViewScan", rel_alias);
1011+
log::trace!("🔍 Relationship '{}' center is NOT ViewScan", rel_alias);
10061012
false
10071013
}
10081014
} else {
@@ -1073,7 +1079,7 @@ impl LogicalPlan {
10731079
}
10741080
}
10751081
} else {
1076-
log::warn!(" ✗ Start node '{}' not found in plan_ctx", start_alias);
1082+
log::trace!(" ✗ Start node '{}' not found in plan_ctx", start_alias);
10771083
}
10781084

10791085
// Expand end node properties
@@ -1103,7 +1109,7 @@ impl LogicalPlan {
11031109
}
11041110
}
11051111
} else {
1106-
log::warn!(" ✗ End node '{}' not found in plan_ctx", end_alias);
1112+
log::trace!(" ✗ End node '{}' not found in plan_ctx", end_alias);
11071113
}
11081114

11091115
// Expand relationship properties (ONLY if not denormalized)
@@ -1140,7 +1146,7 @@ impl LogicalPlan {
11401146
}
11411147
}
11421148
} else {
1143-
log::warn!(" ✗ Relationship '{}' not found in plan_ctx", rel_alias);
1149+
log::trace!(" ✗ Relationship '{}' not found in plan_ctx", rel_alias);
11441150
}
11451151
} else {
11461152
// Denormalized relationship: properties come from end node's table

src/server/bolt_protocol/result_transformer.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ pub fn transform_row(
379379
row.len(),
380380
row.keys().collect::<Vec<_>>()
381381
);
382-
log::info!(
382+
log::trace!(
383383
"🔍 Metadata has {} items: {:?}",
384384
metadata.len(),
385385
metadata
@@ -711,7 +711,7 @@ fn transform_to_path(
711711
// Check for JSON format first (UNION path queries)
712712
// Format: _start_properties, _end_properties, _rel_properties
713713
if row.contains_key("_start_properties") && row.contains_key("_end_properties") {
714-
log::info!("🎯 Detected JSON format for path - parsing _start_properties, _end_properties, _rel_properties");
714+
log::trace!("🎯 Detected JSON format for path - parsing _start_properties, _end_properties, _rel_properties");
715715
return transform_path_from_json(row, start_labels, end_labels, rel_types);
716716
}
717717

@@ -724,7 +724,7 @@ fn transform_to_path(
724724
.first()
725725
.cloned()
726726
.unwrap_or_else(|| "_Unknown".to_string());
727-
log::info!("Creating start node with label: {}", label);
727+
log::trace!("Creating start node with label: {}", label);
728728
create_node_with_label(&label, 0)
729729
});
730730

@@ -735,7 +735,7 @@ fn transform_to_path(
735735
.first()
736736
.cloned()
737737
.unwrap_or_else(|| "_Unknown".to_string());
738-
log::info!("Creating end node with label: {}", label);
738+
log::trace!("Creating end node with label: {}", label);
739739
create_node_with_label(&label, 1)
740740
});
741741

@@ -756,7 +756,7 @@ fn transform_to_path(
756756
.first()
757757
.cloned()
758758
.unwrap_or_else(|| "_UNKNOWN".to_string());
759-
log::info!("Creating relationship with type: {}", rel_type);
759+
log::trace!("Creating relationship with type: {}", rel_type);
760760
create_relationship_with_type(&rel_type, &start_node.element_id, &end_node.element_id)
761761
});
762762

src/server/bolt_protocol/websocket.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,8 @@ impl AsyncRead for WebSocketBoltAdapter {
7979
}
8080
Poll::Ready(Some(Ok(Message::Close(_)))) => {
8181
log::debug!("WebSocket close message received");
82-
Poll::Ready(Err(io::Error::new(
83-
io::ErrorKind::ConnectionAborted,
84-
"WebSocket closed",
85-
)))
82+
// Signal EOF for graceful disconnect instead of error
83+
Poll::Ready(Ok(()))
8684
}
8785
Poll::Ready(Some(Ok(Message::Ping(_)))) | Poll::Ready(Some(Ok(Message::Pong(_)))) => {
8886
// Handled automatically by tungstenite, poll again

0 commit comments

Comments
 (0)