Skip to content

Add CBRT to the V2 engine#1081

Merged
dai-chen merged 2 commits intoopensearch-project:2.xfrom
Bit-Quill:integ-add-cbrt-to-v2
Nov 21, 2022
Merged

Add CBRT to the V2 engine#1081
dai-chen merged 2 commits intoopensearch-project:2.xfrom
Bit-Quill:integ-add-cbrt-to-v2

Conversation

@margarit-h
Copy link
Copy Markdown
Contributor

@margarit-h margarit-h commented Nov 16, 2022 •
edited
Loading

Description

Adds CBRT() math function to the V2 engine.

Usage: CBRT(number) calculates the cube root of a number
Argument type: INTEGER/LONG/FLOAT/DOUBLE
Return type: DOUBLE

(Non-negative) INTEGER/LONG/FLOAT/DOUBLE -> DOUBLE
(Negative) INTEGER/LONG/FLOAT/DOUBLE -> DOUBLE

Example::

opensearchsql> SELECT CBRT(8), CBRT(9.261), CBRT(-27);
fetched rows / total rows = 1/1
+-----------+---------------+-------------+
| CBRT(8)   | CBRT(9.261)   | CBRT(-27)   |
|-----------+---------------+-------------|
| 2.0       | 2.1           | -3.0        |
+-----------+---------------+-------------+

Issues Resolved

[List any issues this PR will resolve]

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.

@margarit-h margarit-h requested a review from a team as a code owner November 16, 2022 23:31
@margarit-h margarit-h mentioned this pull request Nov 16, 2022
Closed
@margarit-h margarit-h force-pushed the integ-add-cbrt-to-v2 branch 2 times, most recently from 126d176 to eb2435a Compare November 17, 2022 19:05
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 17, 2022 •
edited by codecov bot
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.74%. Comparing base (81c9285) to head (08bef27).
⚠️ Report is 477 commits behind head on 2.x.

❌ Your project status has failed because the head coverage (95.74%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files 
@@             Coverage Diff              @@
##                2.x    #1081      +/-   ##
============================================
- Coverage     98.27%   95.74%   -2.54%     
- Complexity     3434     3438       +4     
============================================
  Files           343      353      +10     
  Lines          8540     9206     +666     
  Branches        544      663     +119     
============================================
+ Hits           8393     8814     +421     
- Misses          142      334     +192     
- Partials          5       58      +53
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.28% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MaxKsyunz
MaxKsyunz previously approved these changes Nov 17, 2022
View reviewed changes
acarbonetto
acarbonetto previously approved these changes Nov 17, 2022
View reviewed changes
dai-chen
dai-chen previously approved these changes Nov 17, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Please double check if floating point precision problem may occur in assertion in UT.

@dai-chen dai-chen added the enhancement New feature or request label Nov 17, 2022
@dai-chen
Copy link
Copy Markdown
Collaborator

dai-chen commented Nov 17, 2022

Btw, we may want to add it to PPL grammar and doc too?

@margarit-h
Copy link
Copy Markdown
Contributor Author

margarit-h commented Nov 17, 2022

@dai-chen I am working on a separate PR for adding CBRT() to the PPL.

@margarit-h
Add CBRT to the V2 engine (#166)
75142ab 
Signed-off-by: Margarit Hakobyan <margarith@bitquilltech.com>
@margarit-h margarit-h dismissed stale reviews from dai-chen, acarbonetto, and MaxKsyunz via 75142ab November 17, 2022 23:06
@margarit-h
Fixes after rebase
08bef27 
Signed-off-by: Margarit Hakobyan <margarith@bitquilltech.com>
Yury-Fridlyand
Yury-Fridlyand approved these changes Nov 17, 2022
View reviewed changes
@MaxKsyunz MaxKsyunz requested review from acarbonetto and dai-chen and removed request for acarbonetto, dai-chen, joshuali925 and penghuo November 18, 2022 18:00
@acarbonetto acarbonetto mentioned this pull request Nov 21, 2022
Closed
@dai-chen dai-chen merged commit d4d289c into opensearch-project:2.x Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dai-chen dai-chen dai-chen approved these changes

@acarbonetto acarbonetto acarbonetto left review comments

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

+2 more reviewers

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

@MaxKsyunz MaxKsyunz MaxKsyunz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@margarit-h @codecov-commenter @dai-chen @Yury-Fridlyand @MaxKsyunz @acarbonetto