feat: prepare logical plan to logical plan without params/placeholders #4561
Conversation
github-actions
bot
added
core
| // Test prepare statement from sql to final result | ||
| // This test is equivalent with the test parallel_query_with_filter below but using prepare statement | ||
| #[tokio::test] | ||
| async fn test_prepare_statement() -> Result<()> { |
There was a problem hiding this comment.
I want to have this test to verify my logical plan works correctly
| let expected_dt = "whatever"; | ||
|
|
||
| prepare_stmt_quick_test(sql, expected_plan, expected_dt); | ||
| logical_plan(sql).unwrap(); |
There was a problem hiding this comment.
Instead of calling prepare_stmt_quick_test that will first generate logical plan (and then panic for this test), I call logical_plan directly to simplify the test but still test the same things
| let param_values = vec![ScalarValue::Int32(Some(10))]; | ||
| let expected_plan = "Projection: person.id, person.age\ | ||
| \n Filter: person.age = Int64(10)\ | ||
| \n TableScan: person"; |
There was a problem hiding this comment.
All the available tests for prepare logical plan are used to replace params with values
|
|
||
| #[test] | ||
| #[should_panic(expected = "value: Internal(\"Expected 1 parameters, got 0\")")] | ||
| fn test_prepare_statement_to_plan_one_param_no_value_panic() { |
There was a problem hiding this comment.
And more panics are added for new functions
|
@alamb : this PR is ready for review |
NGA-TRAN
changed the title
There was a problem hiding this comment.
Thank you @NGA-TRAN -- the test coverage looks wonderful to me and the feature works as advertised. I had a few code suggestions that I think would make the code much shorter and simpler to maintain but it would also be ok with me if this PR was merged and we cleaned it up as a follow on PR
There was a problem hiding this comment.
Wow, how amazing your suggestions make the code a lot a lot shorter. I did ask myself yesterday "this recursive replacement must happen a lot and there must be an easy way to do so" but I could not find it and thought the reviewers would pointed them out. And it happen 🎉
I have a addressed them in this PR and will commit shortly
|
@alamb : Thanks for your amazing suggestions. I have addressed them in the PR. It is ready to merge after all the runs finish |
There was a problem hiding this comment.
LGTM -- thanks @NGA-TRAN
FYI @metesynnada @mingmwang as you reviewed other parts of this feature
|
I think this is ready to go. I'll plan to merge it Monday unless anyone else would like more time to review or has other comments |
|
Thanks again @NGA-TRAN |
|
Benchmark runs are scheduled for baseline = d33457c and contender = 4ecf3e7. 4ecf3e7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
apache#4561) * feat: prepare logical plan to logicl plan without params/placeholders * fix: typo * refactor: address review comments * refactor: add index of the params/values into the error message
Which issue does this PR close?
Closes #4550
Rationale for this change
Convert a Prepare Logical Plan into a Logical Plan with all params replaced with values
What changes are included in this PR?
. Recursively walk the logical plan and its expressions to find and replace Placeholders with corresponding values
. Add corresponding tests
Are these changes tested?
Yes
Are there any user-facing changes?
Not yet until this is integrated into physical plan