Support with_parameter and with_parameters in query engine #125
Conversation
leiyuou
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
leiyuou
changed the title
ChunxuTang
left a comment
ChunxuTang
left a comment
There was a problem hiding this comment.
@leiyuou Thanks for the contribution! This is a very nice work.
I carefully re-evaluated the three design options for the parameter support, and I think it's better to go with option 1 (replacing the parameters during the semantic analysis pass). Could you update the PR?
| alt(( | ||
| // $param | ||
| map(preceded(char('$'), identifier), |s| s.to_string()), | ||
| // @param | ||
| map(preceded(char('@'), identifier), |s| s.to_string()), | ||
| // :param | ||
| map(preceded(char(':'), identifier), |s| s.to_string()), | ||
| // {param} | ||
| map(delimited(char('{'), identifier, char('}')), |s| { | ||
| s.to_string() | ||
| }), | ||
| ))(input) |
There was a problem hiding this comment.
Formal Cypher syntax only supports using $ for parameters. Could you remove other special symbols?
| assert "company" in engine_config.node_labels() | ||
|
|
||
|
|
||
| def test_cypher_parameter_syntax(graph_env): |
There was a problem hiding this comment.
The new test is unrelated to the CypherEngine API: maybe put it into the test_graph.py?
| // 1. Resolve parameters during semantic analysis (substitute before planning) | ||
| // 2. Pass parameter map to to_df_value_expr and resolve here | ||
| // 3. Use DataFusion's parameter binding mechanism | ||
| col(format!("${}", name)) |
There was a problem hiding this comment.
@leiyuou I've carefully considered the design options here, and I think it's better to go with option 1 for now. Here are the rationales:
-
If using option 2/3, we can only support parameters in the DataFusion planner, but we also have the simple executor and later Lance native planner. We'll have to re-implement similar mechanisms in all these planners.
-
Very recently, we added a case-insensitivity enforcement feat: implement case-insensitive support #119), which lowers all table/column/alias names. But the parameters here are case-sensitive, which will cause execution failures. It's better to replace them during the semantic analysis step.
3 participants
Support $param, @param, :param, and {param} placeholder in query engine.
Key Changes:
Closes #103.