fix(pg-wire): fix the simple extended query mode #3104
Conversation
ZENOTME
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3104 +/- ##
==========================================
- Coverage 73.64% 73.64% -0.01%
==========================================
Files 736 736
Lines 101570 101606 +36
==========================================
+ Hits 74805 74825 +20
- Misses 26765 26781 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Sorry. I have already updated. |
| .map(|x| { | ||
| Some( | ||
| x.trim_start_matches('\'') | ||
| .trim_end_matches('\'') | ||
| .to_string(), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Add some comments here?
src/utils/pgwire/src/pg_extended.rs
Outdated
| let mut tmp = query_string; | ||
| tmp.push(' '); | ||
| for &i in generic_params.iter() { |
There was a problem hiding this comment.
Use meaningful name like for param in generic_params. And I think .iter() is redudant.
src/utils/pgwire/src/pg_extended.rs
Outdated
| @@ -20,19 +20,47 @@ use regex::Regex; | |||
| use crate::pg_field_descriptor::{PgFieldDescriptor, TypeOid}; | |||
| use crate::pg_protocol::cstr_to_str; | |||
|
|
|||
| fn replace_params(query_string: String, generic_params: &[usize], params: &[Bytes]) -> String { | |||
| fn replace_params(query_string: String, generic_params: &[usize], params: &[String]) -> String { | |||
There was a problem hiding this comment.
You can add PR description as function doc.
src/utils/pgwire/src/pg_extended.rs
Outdated
| tmp = pattern | ||
| .replace_all(&tmp, format!("{}$y", param)) | ||
| .to_string(); | ||
| } | ||
|
|
||
| tmp.pop(); |
There was a problem hiding this comment.
In the line 25, we have tmp.push(' ').
This ' ' helps us to handle the specific case, such as:
'SELECT $2,$1'
Due to '$1' being the last string without any punctuation behind it, it can't be matched by the regex.
To simplify this edge example, I push ' ' to make it be 'SELECT $2,$1 ', now '$1 ' can be matched.
When I finish processing, I pop the final ' '.
There was a problem hiding this comment.
I find another way that is easier to understand.
src/utils/pgwire/src/pg_extended.rs
Outdated
| /// let raw_params = vec!["A".into(), "B".into(), "C".into()]; | ||
| /// let type_description = vec![TypeOid::Varchar; 3]; | ||
| /// let params = parse_params(&type_description, &raw_params); | ||
| /// assert_eq!(params, vec!["\'A\'", "\'B\'", "\'C\'"]) |
There was a problem hiding this comment.
what's the usage of \ ?
There was a problem hiding this comment.
I want to represent escape character, but I find I can directly use ' , I will fix it later.
src/utils/pgwire/src/pg_extended.rs
Outdated
| tmp = pattern | ||
| .replace_all(&tmp, format!("{}$y", param)) | ||
| .to_string(); | ||
| } | ||
| tmp.pop(); | ||
|
|
There was a problem hiding this comment.
looks more clean now, btw delete the empty line
ZENOTME
force-pushed
the
zj/simple_extended_query_fix
branch
2 times, most recently
from
f9f73cf
to
80a4f16
Compare
* modify the parse_params to easier understanding
ZENOTME
force-pushed
the
zj/simple_extended_query_fix
branch
from
80a4f16
to
4b900ec
Compare
What's changed and what's your intention?
fix the simple extended query mode
For the simple extended query mode, it just simply replaces the params. This causes problems when I test in local e2e mode.
For example:
SELECT $1 => SELECT Hello,World ❌ （Expect: SELECT 'Hello,World' ✔）
Then I realized that we couldn't simply replace params, different params need different parse. Such as VARCHAR params need to add (') on both sides of it. So I add the parse_params() to parse the params and now it only supports the VARCHAR type. We can expand another kind in future.
Checklist
./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)
#2924