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

Updated documentation of round function return type #1725

Merged

Conversation

matthewryanwells
Copy link
Contributor

@matthewryanwells matthewryanwells commented Jun 8, 2023

Description

Small fix to incorrect return type in the round function documentation, below in the function implementation in org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java that it should be either returning long or double depending if the first variable was either integer/long or float/double respectively.

private static DefaultFunctionResolver round() {
    return define(BuiltinFunctionName.ROUND.getName(),
        // rand(x)
        impl(nullMissingHandling(v -> new ExprLongValue((long) Math.round(v.integerValue()))),
            LONG, INTEGER),
        impl(nullMissingHandling(v -> new ExprLongValue((long) Math.round(v.longValue()))),
            LONG, LONG),
        impl(nullMissingHandling(v -> new ExprDoubleValue((double) Math.round(v.floatValue()))),
            DOUBLE, FLOAT),
        impl(nullMissingHandling(v -> new ExprDoubleValue(new BigDecimal(v.doubleValue())
                .setScale(0, RoundingMode.HALF_UP).doubleValue())),
            DOUBLE, DOUBLE),

        // rand(x, d)
        impl(nullMissingHandling((x, d) -> new ExprLongValue(new BigDecimal(x.integerValue())
                .setScale(d.integerValue(), RoundingMode.HALF_UP).longValue())),
            LONG, INTEGER, INTEGER),
        impl(nullMissingHandling((x, d) -> new ExprLongValue(new BigDecimal(x.longValue())
                .setScale(d.integerValue(), RoundingMode.HALF_UP).longValue())),
            LONG, LONG, INTEGER),
        impl(nullMissingHandling((x, d) -> new ExprDoubleValue(new BigDecimal(x.floatValue())
                .setScale(d.integerValue(), RoundingMode.HALF_UP).doubleValue())),
            DOUBLE, FLOAT, INTEGER),
        impl(nullMissingHandling((x, d) -> new ExprDoubleValue(new BigDecimal(x.doubleValue())
                .setScale(d.integerValue(), RoundingMode.HALF_UP).doubleValue())),
            DOUBLE, DOUBLE, INTEGER));
  }

Issues Resolved

No connected issue

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.

Signed-off-by: Matthew Wells <[email protected]>
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #1725 (7d65715) into main (f6e2a97) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1725   +/-   ##
=========================================
  Coverage     97.29%   97.29%           
  Complexity     4408     4408           
=========================================
  Files           388      388           
  Lines         10944    10944           
  Branches        774      774           
=========================================
  Hits          10648    10648           
  Misses          289      289           
  Partials          7        7           
Flag Coverage Δ
sql-engine 97.29% <ø> (ø)

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

@Yury-Fridlyand
Copy link
Collaborator

Is it related to #1138?

Copy link
Collaborator

@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.

Nothing changed in docs, but I think you can fix #1138 as far as you're here. It returns DOUBLE in some cases.

Signed-off-by: Matthew Wells <[email protected]>
@matthewryanwells
Copy link
Contributor Author

It has been confirmed that #1138 was not actually a bug

Signed-off-by: Matthew Wells <[email protected]>
@Yury-Fridlyand Yury-Fridlyand merged commit 34cad6e into opensearch-project:main Jun 22, 2023
@Yury-Fridlyand Yury-Fridlyand deleted the round-return-documentation branch June 22, 2023 19:39
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 22, 2023
* fixed round documentation

Signed-off-by: Matthew Wells <[email protected]>
(cherry picked from commit 34cad6e)
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 22, 2023
Yury-Fridlyand pushed a commit that referenced this pull request Jun 22, 2023
* fixed round documentation

Signed-off-by: Matthew Wells <[email protected]>
(cherry picked from commit 34cad6e)

Co-authored-by: Matthew Wells <[email protected]>
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.

3 participants