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.

Sign up for GitHub

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

Jump to bottom

Updated EXPM1() and Tests to New Engine #215

Merged
merged 20 commits into from
Feb 13, 2023
Merged

Updated EXPM1() and Tests to New Engine #215

merged 20 commits into from
Feb 13, 2023

Conversation

matthewryanwells
Copy link

Signed-off-by: Matthew Wells matthew.wells@improving.com

Description

Updated EXPM1 function to run on the new engine, as well as added integration tests and unit tests

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@matthewryanwells
Updated EXPM1() and tests to new engine
e2a6901 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@GumpacG
Copy link

GumpacG commented Feb 2, 2023

Actions are failing due to checkstyle.

Yury-Fridlyand
Yury-Fridlyand reviewed Feb 2, 2023
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What about PPL?

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java Outdated Show resolved Hide resolved
...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java Outdated Show resolved Hide resolved
@GumpacG
Copy link

GumpacG commented Feb 2, 2023

What about PPL?

opensearch-project#1097 for reference

@matthewryanwells
Added EXPM1 to PPL Parser
e357cdd 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@codecov
Copy link

codecov bot commented Feb 2, 2023 •
edited
Loading

Codecov Report

Merging #215 (e5a7001) into integ-expm1 (af188a3) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@                Coverage Diff                @@
##             integ-expm1     #215      +/-   ##
=================================================
- Coverage          98.36%   98.36%   -0.01%     
+ Complexity          3643     3636       -7     
=================================================
  Files                343      343              
  Lines               9017     8990      -27     
  Branches             585      585              
=================================================
- Hits                8870     8843      -27     
  Misses               142      142              
  Partials               5        5
Flag Coverage Δ
sql-engine 98.36% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/main/java/org/opensearch/sql/expression/DSL.java 100.00% <100.00%> (ø)
...h/sql/expression/function/BuiltinFunctionName.java 100.00% <100.00%> (ø)
...ssion/operator/arthmetic/MathematicalFunction.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@matthewryanwells
Added documentation for EXPM1 PPL and fixed small mistake in EXP docu…
c152a95 
…mentation

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Added newline to fix code style issue
b5ab93e 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Copy link
Author

Actions are failing due to checkstyle.

the checkstyle was failing because of the lack of a newline which I have now fixed

@matthewryanwells
Copy link
Author

What about PPL?

opensearch-project#1097 for reference

I have added in PPL, including testing and documentation

@matthewryanwells
removed unnecessary integration tests and fixed typo in another test
2958c3e 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Extracted common code from math functions into external function
3c856a3 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Added javadoc comment to new function
b1c9fde 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Improved Javadoc
457a5a6 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Yury-Fridlyand
Yury-Fridlyand approved these changes Feb 6, 2023
View reviewed changes
Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

You can rebase on upstream/main to make CI pass

@matthewryanwells
Pulling changes from upstream
f013e7d 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Copy link
Author

You can rebase on upstream/main to make CI pass

I have now rebased the changes

@GumpacG
Copy link

GumpacG commented Feb 7, 2023

I would suggest to have the math function base extraction be a part of a different task so that more tests can be included for each of the types that the functions supports. This would prevent the task and PR from becoming large.

@matthewryanwells
Added and improved EXPM1 tests, and improved documentation for CLI an…
6d09e77 
…d PPL

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Removed EXPM1 from PPL
eecd489 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Updated math base function to take any type and included more functio…
8b88d1c 
…ns to use the base

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Made tiny change to documentation
7a028d4 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
reverted euler function to previous iteration as the one I made didn'…
4574275 
…t work

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
GumpacG
GumpacG reviewed Feb 9, 2023
View reviewed changes
docs/user/ppl/functions/math.rst Outdated
@@ -321,7 +321,6 @@ Example::
| 7.38905609893065 |
+------------------+


Copy link

Choose a reason for hiding this comment

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

Accidental deletion?

@matthewryanwells
fixed typo in a comment that I made in a previous commit
62cc771 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
fixed a mistake I made in fixing the typo from my last commit
689a1dc 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Fixed spacing inconsistencies in math.rst file
5a3b724 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
removed the base math function so that it can be pushed in a seperate…
8042ebe 
… pull request

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
Reverted reverted change that removes the refactoring as this will be…
a74ae35 
… done in one commit

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells
updated base function to have more descriptive variables
e5a7001 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells matthewryanwells merged commit b82d5b8 into integ-expm1 Feb 13, 2023
@matthewryanwells matthewryanwells deleted the dev-expm1 branch February 13, 2023 17:25
GumpacG pushed a commit that referenced this pull request Feb 14, 2023
@matthewryanwells
Updated EXPM1() and Tests to New Engine (#215) (opensearch-project#1334)
e990201 
* Updated EXPM1() and tests to new engine

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
GumpacG pushed a commit that referenced this pull request Feb 16, 2023
@opensearch-trigger-bot @matthewryanwells
Updated EXPM1() and Tests to New Engine (#215) (opensearch-project#1334…
42509e1 
…) (opensearch-project#1348)

* Updated EXPM1() and tests to new engine

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
(cherry picked from commit e990201)

Co-authored-by: Matthew Wells <matthew.wells@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GumpacG GumpacG GumpacG approved these changes

@forestmvey forestmvey forestmvey approved these changes

@GabeFernandez310 GabeFernandez310 GabeFernandez310 approved these changes

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

@acarbonetto acarbonetto acarbonetto approved these changes

@margarit-h margarit-h Awaiting requested review from margarit-h

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@matthewryanwells @GumpacG @forestmvey @GabeFernandez310 @Yury-Fridlyand @acarbonetto