Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-957

ROUND should be support INT parameter.

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.0
    • Component/s: None
    • Labels:
      None

      Description

      Currently Tajo returns error the following query.

      default> select round(10);
      ERROR: function round(int4) does not exist
      
      1. TAJO-957_2.patch
        3 kB
        Hyunsik Choi
      2. TAJO-957.patch.txt
        2 kB
        Mai Hai Thanh

        Activity

        Hide
        tajoqa Tajo QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12657544/TAJO-957.patch.txt
        against master revision 72808e0.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

        +1 checkstyle. The patch generated 0 code style errors.

        -1 findbugs. The patch appears to introduce 202 new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in tajo-core.

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/470//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/470//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/470//console

        This message is automatically generated.

        Show
        tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12657544/TAJO-957.patch.txt against master revision 72808e0. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 202 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in tajo-core. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/470//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/470//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/470//console This message is automatically generated.
        Hide
        mhthanh Mai Hai Thanh added a comment -

        Hi Hyoungjun Kim,

        Could you check the submitted patch ?

        Show
        mhthanh Mai Hai Thanh added a comment - Hi Hyoungjun Kim , Could you check the submitted patch ?
        Hide
        hyunsik Hyunsik Choi added a comment -

        Hi Mai,

        Thank you for your contribution. Your fix looks nice. I have one suggestion. Could you move your unit test to TestMathFunctions with the proper name? TestMathFunctions would be a better place to include the unit test. After than, I'll finish the review.

        In addition, in my point of view, there is one more hidden problem. That is a bug (or incorrect implemented) of function finding mechanism which finds the compatible function, even though they are not exactly matched. It can be defined as both functions are compatible if both function arguments can be casted without precision loss. It was implemented in TAJO-215. For example, INT can be casted to FLOAT8 (double) or LONG without precision loss. So, CatalogServer should have found ROUND(FLOAT8) even though ROUND(INT4) function is absent.

        If you want to solve this problem in this issue, you can do it. Otherwise, I'll create another jira issue.

        Show
        hyunsik Hyunsik Choi added a comment - Hi Mai, Thank you for your contribution. Your fix looks nice. I have one suggestion. Could you move your unit test to TestMathFunctions with the proper name? TestMathFunctions would be a better place to include the unit test. After than, I'll finish the review. In addition, in my point of view, there is one more hidden problem. That is a bug (or incorrect implemented) of function finding mechanism which finds the compatible function, even though they are not exactly matched. It can be defined as both functions are compatible if both function arguments can be casted without precision loss. It was implemented in TAJO-215 . For example, INT can be casted to FLOAT8 (double) or LONG without precision loss. So, CatalogServer should have found ROUND(FLOAT8) even though ROUND(INT4) function is absent. If you want to solve this problem in this issue, you can do it. Otherwise, I'll create another jira issue.
        Hide
        mhthanh Mai Hai Thanh added a comment -

        Hi Hyunsik,

        If you feel OK, please help me to move the unit test by yourself. The reason is that I have modified a number of other files in trying to fix some other bugs, so making a new patch for this issue now with git diff is not very convenient.

        For the hidden problem, please create another jira issue. Then I will try to solve it sometime later.

        Show
        mhthanh Mai Hai Thanh added a comment - Hi Hyunsik, If you feel OK, please help me to move the unit test by yourself. The reason is that I have modified a number of other files in trying to fix some other bugs, so making a new patch for this issue now with git diff is not very convenient. For the hidden problem, please create another jira issue. Then I will try to solve it sometime later.
        Hide
        hyunsik Hyunsik Choi added a comment -

        Ok, I'm willing to move the unit test to the proper class, and than I'll upload the patch soon. I'll also create another jira issue for the hidden problem. Thanks!

        Show
        hyunsik Hyunsik Choi added a comment - Ok, I'm willing to move the unit test to the proper class, and than I'll upload the patch soon. I'll also create another jira issue for the hidden problem. Thanks!
        Hide
        hyunsik Hyunsik Choi added a comment -

        +1

        I've verified 'mvn clean install'. As Mai's ask, I've moved the unit test to TestMathFunctions. In addition, I've added round(int8) function. I'll commit it shortly.

        Thanks!

        Show
        hyunsik Hyunsik Choi added a comment - +1 I've verified 'mvn clean install'. As Mai's ask, I've moved the unit test to TestMathFunctions. In addition, I've added round(int8) function. I'll commit it shortly. Thanks!
        Hide
        hyunsik Hyunsik Choi added a comment -

        committed the latest patch to master branch. Thank you for your contribution.

        Show
        hyunsik Hyunsik Choi added a comment - committed the latest patch to master branch. Thank you for your contribution.
        Hide
        mhthanh Mai Hai Thanh added a comment -

        Thank Hyunsik!
        It's my pleasure to contribute to Tajo.

        Show
        mhthanh Mai Hai Thanh added a comment - Thank Hyunsik! It's my pleasure to contribute to Tajo.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #321 (See https://builds.apache.org/job/Tajo-master-build/321/)
        TAJO-957: ROUND should be support INT parameter. (Mai Hai Thanh via hyunsik) (hyunsik: rev 8024f6ab324e9e3ba2c1968386292f9243f44b97)

        • tajo-core/src/main/java/org/apache/tajo/engine/function/math/Round.java
        • CHANGES
        • tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #321 (See https://builds.apache.org/job/Tajo-master-build/321/ ) TAJO-957 : ROUND should be support INT parameter. (Mai Hai Thanh via hyunsik) (hyunsik: rev 8024f6ab324e9e3ba2c1968386292f9243f44b97) tajo-core/src/main/java/org/apache/tajo/engine/function/math/Round.java CHANGES tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java

          People

          • Assignee:
            mhthanh Mai Hai Thanh
            Reporter:
            hjkim Hyoungjun Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development