New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support conditional function CASE
#330
Conversation
Codecov Report
@@ Coverage Diff @@
## main #330 +/- ##
==========================================
+ Coverage 91.66% 91.71% +0.05%
==========================================
Files 137 138 +1
Lines 8383 8434 +51
==========================================
+ Hits 7684 7735 +51
Misses 699 699
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an issue that a lot more to consider, but you really did well, thanks!
There are a few comments.
It looks that it's a good time to get familar with StreamExt
and TryStreamExt
.
https://docs.rs/futures/0.3.16/futures/stream/trait.StreamExt.html
https://docs.rs/futures/0.3.16/futures/stream/trait.TryStreamExt.html
Can you tell me the current status of this PR? |
I am currently working on checking whether the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single comment
src/executor/evaluate/mod.rs
Outdated
let (when_then, else_result) = match when_then | ||
.iter() | ||
.map(|(_, then)| match then { | ||
Evaluated::Literal(l) => Some(l), | ||
_ => None, | ||
}) | ||
.collect::<Option<Vec<_>>>() | ||
{ | ||
None => { | ||
let when_then_value = when_then | ||
.into_iter() | ||
.map(|(when, then)| -> Result<(Value, Value)> { | ||
match (when.try_into(), then.try_into()) { | ||
(Ok(w), Ok(t)) => Ok((w, t)), | ||
(Err(e), _) | (_, Err(e)) => Err(e), | ||
} | ||
}) | ||
.collect::<Result<Vec<(Value, Value)>>>()?; | ||
|
||
let wt_type = when_then_value | ||
.iter() | ||
.map(|(_, then)| discriminant(then)) | ||
.collect_vec(); | ||
|
||
if !wt_type.iter().all_equal() { | ||
return Err(EvaluateError::UnequalResultTypes("CASE".to_owned()).into()); | ||
} | ||
let else_result = match else_result { | ||
Some(Evaluated::Value(cv)) => { | ||
let er = cv.into_owned(); | ||
if Some(&discriminant(&er)) != wt_type.first() { | ||
return Err( | ||
EvaluateError::UnequalResultTypes("CASE".to_owned()).into() | ||
); | ||
} | ||
Some(Evaluated::from(er)) | ||
} | ||
_ => else_result, | ||
}; | ||
|
||
( | ||
when_then_value | ||
.into_iter() | ||
.map(|(when, then)| (Evaluated::from(when), Evaluated::from(then))) | ||
.collect::<Vec<_>>(), | ||
else_result, | ||
) | ||
} | ||
Some(wt_type) => { | ||
let wt_type = wt_type.into_iter().map(discriminant).collect_vec(); | ||
if !wt_type.iter().all_equal() | ||
|| match &else_result { | ||
Some(Evaluated::Literal(er)) => { | ||
Some(&discriminant(er)) != wt_type.first() | ||
} | ||
_ => false, | ||
} | ||
{ | ||
return Err(EvaluateError::UnequalResultTypes("CASE".to_owned()).into()); | ||
} | ||
|
||
(when_then, else_result) | ||
} | ||
}; | ||
|
||
Ok(when_then | ||
.into_iter() | ||
.find_map(|(when, then)| when.eq(&operand).then(|| then)) | ||
.unwrap_or_else(|| match else_result { | ||
Some(er) => er, | ||
None => Evaluated::from(Value::Null), | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literal
and Value
can be mixed in then
, so
let (when_then, else_result) = match when_then
.iter()
.map(|(_, then)| match then {
Evaluated::Literal(l) => Some(l),
_ => None,
})
.collect::<Option<Vec<_>>>()
this above looks unnecessary.
fyi.
if !when_then
.iter()
.map(|(_, then)| then.try_into().map(discriminant))
.all_equal()
{
return Err(EvaluateError::UnequalResultTypes("CASE".to_owned()).into());
}
for (when, then) in when_then.into_iter() {
if when == operand {
return Ok(then);
}
}
Ok(else_result.unwrap_or_else(|| Evaluated::from(Value::Null)))
This is still not complete code, but it passes the current test case
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literal
andValue
can be mixed inthen
, solet (when_then, else_result) = match when_then .iter() .map(|(_, then)| match then { Evaluated::Literal(l) => Some(l), _ => None, }) .collect::<Option<Vec<_>>>()this above looks unnecessary.
They can be mixed, but then we have to use try_into()
for every then
values, no matter the enum variance. When every then
values are Evaluated::Literal
type, there's no need to use try_into()
so I thought it would be better split the conditions as:
- When all the
then
values areEvaluated::Literal(l)
-> Get the variant valuel
and type check - When there's at least one
Evaluated::Value(v)
-> Squeeze the value out withtry_into()
and type check
Well, I'd be glad if using try_into()
for every case isn't a bad idea. Because my logic here has produced bunch of ugly condition branched codes :< Please let me know if there's something wrong about my opinion 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. ok now I got why you wanted to handle Literal
separately, thanks for sharing the details.
Then.. let's think about merge these then
comparing logic into a single statement :)
How about using window function?
Rather than using all_equal
in here, we can use try_fold
for the alternative for window.
Or.. simply like this
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=94efcd6520aff2e0eb553c3ab95f077d
Then, we can handle comparing Literal
- Value
, Value
- Valueand
Literal-
Literal` all together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I'll try shrinking code a lot 📄
src/executor/evaluate/mod.rs
Outdated
if let Some((_, then)) = stream::iter(when_then.iter()) | ||
.map(Ok::<_, Error>) | ||
.and_then(|(when, then)| async move { Ok((eval(when).await?, eval(then).await?)) }) | ||
.try_collect::<Vec<(Evaluated, Evaluated)>>() | ||
.await? | ||
.into_iter() | ||
.find(|(when, _)| when.eq(&operand)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we don't even need to evaluate
, so
for (when, then) in when_then.iter() {
let when = eval(when).await?;
if when.eq(&operand) {
return eval(then).await;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Thanks!
src/executor/evaluate/error.rs
Outdated
|
||
#[error("the result types should be equal: {0}")] | ||
UnequalResultTypes(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks the only left thing to modify.
Let's remove this and merge this to the main!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my, totally right! I'll do it right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate and respect your effort.
It was long but meaningful journey.
Awesome, merging!!
@panarch Hooray!! Thanks for all advices. I've learned so much from this! |
Resolves issue #140
I added some codes to support conditional functional
CASE
.The
CASE
expression evaluates a list of conditions and returns an expression based on the result of the evaluation.It is similar to the
IF-THEN-ELSE
statement in other programming languages.Basic structure of the
CASE
function/* expressions in [ ] are empty-able */ CASE [ {case_expression} ] WHEN {condition_1} THEN {result_1} WHEN {condition_2} THEN {result_2} ... WHEN {condition_n} THEN {result_n} [ ELSE {else_result} ] END;
{case_expression}
is empty,CASE
performssearched_case
, otherwisesimple_case
.ELSE {else_result}
is empty, it will returnNull
when it meets the condition.Use cases
case_expression
. It will match withcondition
and returnresult
.case_expression
. It will returnresult
only for truecondition
.References
CASE
: https://www.sqlitetutorial.net/sqlite-case/CASE
: https://www.postgresql.org/docs/current/functions-conditional.html#FUNCTIONS-CASE