Implement substr function #341
Conversation
Codecov Report
@@ Coverage Diff @@
## main #341 +/- ##
==========================================
+ Coverage 91.20% 91.25% +0.05%
==========================================
Files 126 127 +1
Lines 7933 8005 +72
==========================================
+ Hits 7235 7305 +70
- Misses 698 700 +2
Continue to review full report at Codecov.
|
src/translate/function.rs
Outdated
| let count = if args.len() == 2 { | ||
| None | ||
| } else { | ||
| Some(translate_expr(args[2])?) | ||
| }; |
There was a problem hiding this comment.
fyi. Let's get familiar with using methods provided by bool and Option :)
https://doc.rust-lang.org/std/primitive.bool.html#method.then
https://doc.rust-lang.org/std/option/enum.Option.html#method.transpose
let count = (args.len() > 2)
.then(|| translate_expr(args[2]))
.transpose()?;There was a problem hiding this comment.
Thanks for the information.
Edited as you said.
src/executor/evaluate/mod.rs
Outdated
| let count = match count { | ||
| Some(expr) => match eval_to_integer(expr).await? { | ||
| Nullable::Value(v) => match v { | ||
| x if x < 0 => return Err(EvaluateError::NegativeSubstrLenNotAllowed.into()), | ||
| _ => v, | ||
| }, | ||
| Nullable::Null => -1, | ||
| }, | ||
| None => -1, | ||
| }; |
There was a problem hiding this comment.
Current count variable has two independent roles
- whether
countexists or not - number of chars to substr.
ast already properly provides as a form of Option<Expr> and you'll be able to convert it into Option<usize>.
Then, you can handle Option<usize> directly without merging two info into i64 type.
There was a problem hiding this comment.
I don't know if it's okay to change type of count to Option<usize>,, 😹
Because, to calculate the variable e, count may have to be added to the start, which causes type casting.
Is there any simpler way? XD
src/tests/function/substr.rs
Outdated
| r#"SELECT SUBSTR("ABC", -1, NULL) AS test FROM SingleItem"#, | ||
| Ok(select!( | ||
| "test" | ||
| Str; | ||
| "ABC".to_owned() | ||
| )), |
There was a problem hiding this comment.
It is supposed to be NULL if any param value is NULL.
There was a problem hiding this comment.
Thanks! I'll change TC to return NULL!
src/executor/evaluate/mod.rs
Outdated
| let start = match eval_to_integer(start).await? { | ||
| Nullable::Value(v) => v, | ||
| Nullable::Null => return Ok(Evaluated::from(Value::Null)), | ||
| }; | ||
|
|
||
| let count = match count { | ||
| Some(expr) => match eval_to_integer(expr).await? { | ||
| Nullable::Value(v) => match v { | ||
| x if x < 0 => return Err(EvaluateError::NegativeSubstrLenNotAllowed.into()), | ||
| _ => Some(v), | ||
| }, | ||
| Nullable::Null => None, | ||
| }, | ||
| None => None, | ||
| }; | ||
|
|
||
| let s: usize = if start <= 0 { 0 } else { (start - 1) as usize }; | ||
| let e = match count { | ||
| Some(v) => { | ||
| if (start - 1 + v) < 0 { | ||
| 0 | ||
| } else if (start - 1 + v) <= string.len() as i64 { | ||
| (start - 1 + v) as usize | ||
| } else { | ||
| string.len() | ||
| } | ||
| } | ||
| None => string.len(), | ||
| }; | ||
|
|
||
| let result = if s >= string.len() { | ||
| String::from("") | ||
| } else { | ||
| String::from(&string[s..e]) | ||
| }; |
There was a problem hiding this comment.
Ok, here type casting to usize looks inevitable.
However, we can simplify the codes by doing a bit of rearrangement.
std::cmp::{min, max} would be useful.
let start = match eval_to_integer(start).await? {
Nullable::Value(v) => v - 1,
Nullable::Null => {
return Ok(Evaluated::from(Value::Null));
}
};
let end = match count {
Some(expr) => match eval_to_integer(expr).await? {
Nullable::Value(count) => {
if count < 0 {
return Err(EvaluateError::NegativeSubstrLenNotAllowed.into());
}
min(max(start + count, 0) as usize, string.len())
}
Nullable::Null => {
return Ok(Evaluated::from(Value::Null));
}
},
None => string.len(),
};
let start = min(max(start, 0) as usize, string.len());
let string = String::from(&string[start..end]); There was a problem hiding this comment.
I mainly focused on two points.
- handling
startcodes were splitted into two.
let s: usize = if start <= 0 { 0 } else { (start - 1) as usize };let result = if s >= string.len() {
String::from("")
} else {
String::from(&string[s..e])
};- Also the
counthandling match codes
let count = match count {
Some(expr) => match eval_to_integer(expr).await? {
..let e = match count {
Some(v) => {
if (start - 1 + v) < 0 {
..There was a problem hiding this comment.
Thanks for kindly telling me how to refactor.:innocent:
I couldn't think I could turn it into a simple code like you wrote.
I think I got a little closer to rust thanks to your help 😋
|
I think this PR is ready for re-review 🤣 Please take a look. Thanks ! |
Resolve #293
substr ( string text, start integer [, count integer ] ) → text
Extracts the substring of string starting at the start'th character, and extending for count characters if that is specified. (Same as substring(string from start for count).