Updated documentation of round function return type by matthewryanwells · Pull Request #1725 · opensearch-project/sql · GitHub
Skip to content

Updated documentation of round function return type#1725

Merged
Yury-Fridlyand merged 3 commits into
opensearch-project:mainfrom
Bit-Quill:round-return-documentation
Jun 22, 2023
Merged

Updated documentation of round function return type#1725
Yury-Fridlyand merged 3 commits into
opensearch-project:mainfrom
Bit-Quill:round-return-documentation

Conversation

@matthewryanwells

@matthewryanwells matthewryanwells commented Jun 8, 2023 •
edited by Yury-Fridlyand
Loading

Copy link
Copy Markdown
Contributor

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.

@matthewryanwells
fixed round docuementation
bf13566 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@codecov

codecov Bot commented Jun 8, 2023 •
edited
Loading

Copy link
Copy Markdown

@Yury-Fridlyand

Yury-Fridlyand commented Jun 8, 2023

Copy link
Copy Markdown
Collaborator

Is it related to #1138?

Yury-Fridlyand
Yury-Fridlyand reviewed Jun 8, 2023
View reviewed changes

@Yury-Fridlyand Yury-Fridlyand left a comment

Copy link
Copy Markdown
Collaborator

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.

@matthewryanwells
fixed long instead of double
bcec83d 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@matthewryanwells

matthewryanwells commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

@matthewryanwells
merging upstream changes
7d65715 
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@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
@matthewryanwells @github-actions
Updated documentation of round function return type (#1725)
934ebd0 
* fixed round documentation

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
(cherry picked from commit 34cad6e)
@opensearch-trigger-bot opensearch-trigger-bot Bot mentioned this pull request Jun 22, 2023
Merged
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 22, 2023
@matthewryanwells @acarbonetto
Updated documentation of round function return type (opensearch-proje…
e02b449 
…ct#1725)

* fixed round documentation

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Yury-Fridlyand pushed a commit that referenced this pull request Jun 22, 2023
@opensearch-trigger-bot @matthewryanwells
Updated documentation of round function return type (#1725) (#1770)
61c8691 
* fixed round documentation

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

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

@pjfitzgibbons pjfitzgibbons Awaiting requested review from pjfitzgibbons
@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner
@kavithacm kavithacm Awaiting requested review from kavithacm
@derek-ho derek-ho Awaiting requested review from derek-ho
@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner
@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner
@YANG-DB YANG-DB Awaiting requested review from YANG-DB
@rupal-bq rupal-bq Awaiting requested review from rupal-bq
@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner
@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner
@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner
@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner
@seankao-az seankao-az Awaiting requested review from seankao-az
@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz
@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner
@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner
@GumpacG GumpacG Awaiting requested review from GumpacG
2 more reviewers
@forestmvey forestmvey forestmvey approved these changes
@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport 2.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@matthewryanwells @Yury-Fridlyand @forestmvey