feature: add support for variable re-use in MATCH clause #126
Conversation
|
@beinan @ChunxuTang could you PTAL? Curious why are we passing query in |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
beinan
left a comment
beinan
left a comment
There was a problem hiding this comment.
Great contribution! just left some minor comments. @ChunxuTang do you want to take a look also?
| property: self | ||
| .config | ||
| .get_node_mapping(&target_label) | ||
| .unwrap() |
There was a problem hiding this comment.
unwrap() will panic if the label exists in the query but isn’t present in the mapping (or
if lookup is case‑sensitive and misses). Can we return a PlanError instead (or fall back to default_node_id_field)? This is a hard
crash in planning.
|
|
||
| impl LogicalPlanner { | ||
| pub fn new() -> Self { | ||
| pub fn new(config: GraphConfig) -> Self { |
There was a problem hiding this comment.
this is a breaking change @ChunxuTang do you think it's ok?
| .clone() | ||
| .unwrap_or_else(|| format!("_node_{}", self.variables.len())); | ||
| // Determine target variable and check for reuse | ||
| // TODO: create error types for these cases |
There was a problem hiding this comment.
we can fix in another pr. would it be better to define dedicated error variants for reuse issues (unlabeled reuse, label mismatch, rel reuse)? Makes them easier to test/match and avoids string matching.
|
|
||
| // Phase 2: Graph Logical Plan | ||
| let mut logical_planner = LogicalPlanner::new(); | ||
| let mut logical_planner = LogicalPlanner::new(config.clone()); |
There was a problem hiding this comment.
nit: adds another clone of GraphConfig. Probably fine, but if we want to avoid extra clones, we could accept &GraphConfig or Arc in the planner.
Because the planner is stateful across a single plan run, not across multiple queries. plan(&CypherQuery) keeps the planner reusable and makes it clear that the query is per‑call input. The constructor is used for long‑lived config/state (like GraphConfig or counters), while the query is a transient input. If we put the query in the constructor, we’d either (a) need a new planner per query anyway, or (b) store the query inside and prevent reusing the planner for another query. Passing it to plan is the more flexible pattern and matches how SemanticAnalyzer/DataFusionPlanner are used. |
|
@beinan Per discussion in this thread #111 (comment), I think it's better to reject self-reference queries and encourage users to rewrite the query to fit better for columnar storage. In the longer term, it's better to have a query rewritter component to automatically rewrite self-reference queries. |
4 participants
This PR adds support for re-using variables in a MATCH clause with caveats below:
Used label id filter on the nodes to achieve re-use
Added unit and system tests
Closes: #111