Skip to content
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

Merged
merged 49 commits into from Oct 9, 2021
Merged

Conversation

MRGRAVITY817
Copy link
Collaborator

@MRGRAVITY817 MRGRAVITY817 commented Aug 27, 2021

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;
  • If {case_expression} is empty, CASE performs searched_case, otherwise simple_case.
  • If ELSE {else_result} is empty, it will return Null when it meets the condition.

Use cases

  1. Simple Case: Using case with case_expression. It will match with condition and return result.
SELECT CASE id         /* CASE case_expression */
  WHEN 1 THEN "Tony"   /* WHEN condition THEN result */
  WHEN 2 THEN "Steve"
  WHEN 3 THEN "Natasha"
  ELSE "Bruce"        /* if nothing matches, default */
END;
  1. Searched Case: Using case without case_expression. It will return result only for true condition.
SELECT CASE                  /* CASE case_expression */
  WHEN id = 1 THEN "Tony"    /* WHEN condition THEN result */
  WHEN id = 2 THEN "Steve"
  WHEN id = 3 THEN "Natasha"
  ELSE "Bruce"               /* if nothing matches, default */
END;

References

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #330 (5e31d34) into main (0880d0a) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/tests/mod.rs 100.00% <ø> (ø)
src/executor/evaluate/mod.rs 95.25% <100.00%> (+0.21%) ⬆️
src/tests/case.rs 100.00% <100.00%> (ø)
src/translate/expr.rs 98.75% <100.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0880d0a...5e31d34. Read the comment docs.

@panarch panarch self-requested a review August 27, 2021 11:22
@panarch panarch added the enhancement New feature or request label Aug 27, 2021
src/ast/expr.rs Outdated Show resolved Hide resolved
@MRGRAVITY817 MRGRAVITY817 marked this pull request as ready for review August 28, 2021 15:15
@MRGRAVITY817 MRGRAVITY817 marked this pull request as draft August 28, 2021 15:19
src/translate/expr.rs Outdated Show resolved Hide resolved
@MRGRAVITY817 MRGRAVITY817 marked this pull request as ready for review August 28, 2021 16:03
Copy link
Member

@panarch panarch left a 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

src/translate/expr.rs Outdated Show resolved Hide resolved
src/translate/expr.rs Outdated Show resolved Hide resolved
src/translate/expr.rs Outdated Show resolved Hide resolved
src/translate/expr.rs Outdated Show resolved Hide resolved
src/executor/evaluate/mod.rs Outdated Show resolved Hide resolved
src/executor/evaluate/mod.rs Outdated Show resolved Hide resolved
@ever0de
Copy link
Member

ever0de commented Sep 23, 2021

Can you tell me the current status of this PR?
Can I do a review?

@MRGRAVITY817
Copy link
Collaborator Author

Can you tell me the current status of this PR?
Can I do a review?

I am currently working on checking whether the THEN value from WHEN THEN is an Evaluated::Literal or Evaluated::Value. But I'd gladly have any helpful reviews 😄 since I've started this just now

Copy link
Member

@ever0de ever0de left a 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/error.rs Outdated Show resolved Hide resolved
@MRGRAVITY817 MRGRAVITY817 marked this pull request as draft September 27, 2021 01:51
Comment on lines 196 to 267
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),
}))
Copy link
Member

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.

Copy link
Collaborator Author

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.

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:

  1. When all the then values are Evaluated::Literal(l) -> Get the variant value l and type check
  2. When there's at least one Evaluated::Value(v) -> Squeeze the value out with try_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 👍

Copy link
Member

@panarch panarch Sep 27, 2021

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 - ValueandLiteral-Literal` all together.

Copy link
Collaborator Author

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 📄

@MRGRAVITY817 MRGRAVITY817 marked this pull request as ready for review October 5, 2021 08:20
Comment on lines 183 to 189
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))
Copy link
Member

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;
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. Thanks!

@panarch panarch self-requested a review October 9, 2021 15:34
Comment on lines 60 to 62

#[error("the result types should be equal: {0}")]
UnequalResultTypes(String),
Copy link
Member

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!

Copy link
Collaborator Author

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

@panarch panarch self-requested a review October 9, 2021 15:48
Copy link
Member

@panarch panarch left a 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 panarch merged commit e0a67a9 into gluesql:main Oct 9, 2021
@MRGRAVITY817
Copy link
Collaborator Author

@panarch Hooray!! Thanks for all advices. I've learned so much from this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants