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

Add functions: ASIN(), ACOS(), ATAN() #296

Merged
merged 11 commits into from
Aug 28, 2021
Merged

Add functions: ASIN(), ACOS(), ATAN() #296

merged 11 commits into from
Aug 28, 2021

Conversation

boomkim
Copy link
Contributor

@boomkim boomkim commented Aug 18, 2021

Add ASIN(), ACOS(), ATAN() functions.
These functions support numeric string literals and boolean and floating numbers.
resolve #285

referenced @maruoovv 's pull request a lot.

@panarch panarch self-requested a review August 18, 2021 10:13
@panarch panarch added the enhancement New feature or request label Aug 18, 2021
@boomkim
Copy link
Contributor Author

boomkim commented Aug 18, 2021

Let me fix clippy error and pull request again.

@boomkim boomkim closed this Aug 18, 2021
@boomkim
Copy link
Contributor Author

boomkim commented Aug 18, 2021

Resolved clippy error.

@boomkim boomkim reopened this Aug 18, 2021
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.

Changes are simple and straightforward to look around.
Only change requests are about the specifications.
Thanks! 👍 👍 👍

Comment on lines 48 to 50

#[error("out of range: {0}")]
OutOfRange(String),
Copy link
Member

Choose a reason for hiding this comment

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

Ok, here let's simply depend on Rust compiler.
Rust compiler will return NaN and we can use that rather than returning explicit error.

Comment on lines 288 to 294
Value::F64(v) => Some(v),
Value::Str(v) => match f64::from_str(&v) {
Ok(f) => Some(f),
Err(_) => None,
},
Value::I64(v) => f64::from_i64(v),
_ => None,
Copy link
Member

Choose a reason for hiding this comment

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

Now, let's make the spec clear.

You can only accept three types; F64, I64 and Null.
Except for these three types, FunctionReuqiresFloatValue can be the one to return.
F64 - we can use directly.
I64 - as f64 to cast and we can use that.
Null -> then return value becomes also null.

@panarch panarch self-requested a review August 26, 2021 15:58
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021

Codecov Report

Merging #296 (0f82ab6) into main (23e0e87) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
+ Coverage   90.95%   91.01%   +0.06%     
==========================================
  Files         126      126              
  Lines        7714     7770      +56     
==========================================
+ Hits         7016     7072      +56     
  Misses        698      698              
Impacted Files Coverage Δ
src/tests/mod.rs 100.00% <ø> (ø)
src/executor/evaluate/mod.rs 93.46% <100.00%> (+0.19%) ⬆️
src/tests/function/math_function.rs 100.00% <100.00%> (ø)
src/translate/function.rs 100.00% <100.00%> (ø)

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 23e0e87...0f82ab6. Read the comment docs.

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.

There's only a single comment and everything else looks really good 👍
You can think in more simple way cause now we have eval_to_float.

Comment on lines 332 to 344
let number = match eval(expr).await?.try_into()? {
Value::F64(v) => Some(v),
Value::I64(v) => f64::from_i64(v),
Value::Null => return Ok(Evaluated::from(Value::Null)),
_ => None,
};

match number {
Some(v) => Ok(Evaluated::from(Value::F64(v.asin()))),
None => Err(
EvaluateError::FunctionRequiresFloatOrIntegerValue("ASIN".to_owned()).into(),
),
}
Copy link
Member

@panarch panarch Aug 26, 2021

Choose a reason for hiding this comment

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

Behavior really looks good.
For this, let's use eval_to_float closure which is used by all other functions to take float values.
eval_to_float can provide consistent behavior through float param type functions.

You can apply this to ASin, ACos and ATan.

@panarch panarch self-requested a review August 28, 2021 04:01
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.

Everything looks great, thanks! 👍 👍 👍

@panarch panarch merged commit 846f556 into gluesql:main Aug 28, 2021
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.

Implement Function: ASIN, ACOS, ATAN
3 participants