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 sin cos etc trigonometric function in sql #7182

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

xueyumusic
Copy link
Contributor

This PR tries to support sin, cos, tan, cot, asin, acos, atan, atan2 etc function and pi constant in sql based on the current calcite framework. They are common math functions.

Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

@xueyumusic, could you please test this patch against the latest master (it uses EXPRESSION_POST_AGG, which has been renamed recently) and also add docs to math-expr.md and sql.md for the new functions?

Other than those things and the line comments, this looks good to me.

throw new IAE("Function[%s] needs 0 argument", name());
}

return ExprEval.of(Math.PI);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be extracted to a private static final constant.

.context(QUERY_CONTEXT_DONT_SKIP_EMPTY_BUCKETS)
.build()),
ImmutableList.of(
new Object[]{11.0, Math.sin(Math.PI / 6), Math.cos(Math.PI / 6), Math.tan(Math.PI / 6), Math.cos(Math.PI / 6) / Math.sin(Math.PI / 6),
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to put one result on each line to satisfy the style checker.

@xueyumusic
Copy link
Contributor Author

Hi, @gianm , Thanks a lot for review. I have rebased on master and updated the codes, thank you.

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

👍 thank you!

@gianm gianm merged commit 6511827 into apache:master Mar 5, 2019
@jihoonson jihoonson added this to the 0.15.0 milestone May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants