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

RoundFloat8 should return Float8Datum type.

    Details

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

      Description

      Currently RoundFloat8 function returns TextDatum. So the following query occurs InvalidOperationException.

       select * from (
          select n_nationkey as key, 
                    case when n_nationkey < 6 
                        then round((n_nationkey * 100 / 2.123)::float4 / (n_regionkey + 1)::float4, 2) 
                        else 100.0 
                    end as val 
            from nation) a 
      order by a.key
      
      2014-07-25 16:40:08,418 ERROR: org.apache.tajo.worker.Task (run(432)) - Cannot compare to FLOAT8 type datum
      org.apache.tajo.exception.InvalidOperationException: Cannot compare to FLOAT8 type datum
      	at org.apache.tajo.datum.TextDatum.compareTo(TextDatum.java:113)
      	at org.apache.tajo.storage.TableStatistics.analyzeField(TableStatistics.java:91)
      	at org.apache.tajo.storage.RawFile$RawFileAppender.addTuple(RawFile.java:600)
      	at org.apache.tajo.engine.planner.physical.RangeShuffleFileWriteExec.next(RangeShuffleFileWriteExec.java:101)
      	at org.apache.tajo.worker.Task.run(Task.java:425)
      	at org.apache.tajo.worker.TaskRunner$1.run(TaskRunner.java:406)
      	at java.lang.Thread.run(Thread.java:744)
      

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #321 (See https://builds.apache.org/job/Tajo-master-build/321/)
        TAJO-978: RoundFloat8 should return Float8Datum type. (Hyoungjun Kim via hyunsik) (hyunsik: rev 650157c5907c43a2f9228cd9a3ff59619e24efb6)

        • tajo-core/src/test/resources/queries/TestSelectQuery/testCaseWhenRound.sql
        • tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java
        • tajo-core/src/test/resources/results/TestSelectQuery/testCaseWhenRound.result
        • tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java
        • CHANGES
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #321 (See https://builds.apache.org/job/Tajo-master-build/321/ ) TAJO-978 : RoundFloat8 should return Float8Datum type. (Hyoungjun Kim via hyunsik) (hyunsik: rev 650157c5907c43a2f9228cd9a3ff59619e24efb6) tajo-core/src/test/resources/queries/TestSelectQuery/testCaseWhenRound.sql tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java tajo-core/src/test/resources/results/TestSelectQuery/testCaseWhenRound.result tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java CHANGES
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/tajo/pull/96

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/96
        Hide
        hyunsik Hyunsik Choi added a comment -

        committed. Thank you for your contribution!

        Show
        hyunsik Hyunsik Choi added a comment - committed. Thank you for your contribution!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/96#issuecomment-50852178

        +1

        The patch looks good to me. Even tough this patch includes dividing by zero problem, infinity, and NaN issues in real values, you seem to be going to solve it in TAJO-979 (https://issues.apache.org/jira/browse/TAJO-979). Otherwise, please the problems in TAJO-979.

        Thanks!

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/96#issuecomment-50852178 +1 The patch looks good to me. Even tough this patch includes dividing by zero problem, infinity, and NaN issues in real values, you seem to be going to solve it in TAJO-979 ( https://issues.apache.org/jira/browse/TAJO-979 ). Otherwise, please the problems in TAJO-979 . Thanks!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user babokim commented on the pull request:

        https://github.com/apache/tajo/pull/96#issuecomment-50566282

        I applied your comments. Please review. Thanks.

        Show
        githubbot ASF GitHub Bot added a comment - Github user babokim commented on the pull request: https://github.com/apache/tajo/pull/96#issuecomment-50566282 I applied your comments. Please review. Thanks.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user babokim commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15564108

        — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java —
        @@ -470,4 +470,40 @@ public final void testNowInMultipleTasks() throws Exception

        { executeString("DROP TABLE table11 PURGE"); }

        }
        +
        + @Test
        + public void testCaseWhenRound() throws Exception {
        — End diff –

        I'll add sql and result file.

        Show
        githubbot ASF GitHub Bot added a comment - Github user babokim commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15564108 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java — @@ -470,4 +470,40 @@ public final void testNowInMultipleTasks() throws Exception { executeString("DROP TABLE table11 PURGE"); } } + + @Test + public void testCaseWhenRound() throws Exception { — End diff – I'll add sql and result file.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user babokim commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15564099

        — Diff: tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java —
        @@ -428,19 +430,27 @@ public void testPi() throws IOException {

        @Test
        public void testRoundWithSpecifiedPrecision() throws IOException {
        + // divide zero
        + try {
        + testSimpleEval("select round(10.0/0.0,2) ", new String[]

        {""}

        );
        + fail("10.0/0 should throw InvalidOperationException");
        + } catch (InvalidOperationException e)

        { + //success + }

        +
        testSimpleEval("select round(42.4382,2) ", new String[]

        {"42.44"}

        );
        testSimpleEval("select round(-42.4382,2) ", new String[]

        {"-42.44"}

        );

        • testSimpleEval("select round(-425,2) ", new String[] {"-425"}

          );

        • testSimpleEval("select round(425,2) ", new String[] {"425"}

          );
          + testSimpleEval("select round(-425,2) ", new String[]

          {"-425.0"}

          );
          + testSimpleEval("select round(425,2) ", new String[]

          {"425.0"}

          );

        • testSimpleEval("select round(1234567890,0) ", new String[] {"1234567890"});
          - testSimpleEval("select round(1234567890,1) ", new String[]{"1234567890"}

          );

        • testSimpleEval("select round(1234567890,2) ", new String[] {"1234567890"}

          );
          + testSimpleEval("select round(1234567890,0) ", new String[]

          {"1.23456789E9"});
          + testSimpleEval("select round(1234567890,1) ", new String[]{"1.23456789E9"}

          );
          + testSimpleEval("select round(1234567890,2) ", new String[]

          {"1.23456789E9"}

          );

        testSimpleEval("select round(1.2345678901234567,13) ", new String[]

        {"1.2345678901235"}

        );

        • testSimpleEval("select round(1234567890.1234567,3) ", new String[] {"1234567890.123"}

          );

        • testSimpleEval("select round(1234567890.1234567,5) ", new String[] {"1234567890.12346"}

          );

        • //testSimpleEval("select round(1234567890.1234567890,7) ", new String[] {"1234567890.1234568"});
          + testSimpleEval("select round(1234567890.1234567,3) ", new String[]{"1.234567890123E9"});
          + testSimpleEval("select round(1234567890.1234567,5) ", new String[]{"1.23456789012346E9"});
          +//testSimpleEval("select round(1234567890.1234567890,7) ", new String[]{"1234567890.1234568"}

          );

            • End diff –

        I'll remove.

        Show
        githubbot ASF GitHub Bot added a comment - Github user babokim commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15564099 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java — @@ -428,19 +430,27 @@ public void testPi() throws IOException { @Test public void testRoundWithSpecifiedPrecision() throws IOException { + // divide zero + try { + testSimpleEval("select round(10.0/0.0,2) ", new String[] {""} ); + fail("10.0/0 should throw InvalidOperationException"); + } catch (InvalidOperationException e) { + //success + } + testSimpleEval("select round(42.4382,2) ", new String[] {"42.44"} ); testSimpleEval("select round(-42.4382,2) ", new String[] {"-42.44"} ); testSimpleEval("select round(-425,2) ", new String[] {"-425"} ); testSimpleEval("select round(425,2) ", new String[] {"425"} ); + testSimpleEval("select round(-425,2) ", new String[] {"-425.0"} ); + testSimpleEval("select round(425,2) ", new String[] {"425.0"} ); testSimpleEval("select round(1234567890,0) ", new String[] {"1234567890"}); - testSimpleEval("select round(1234567890,1) ", new String[]{"1234567890"} ); testSimpleEval("select round(1234567890,2) ", new String[] {"1234567890"} ); + testSimpleEval("select round(1234567890,0) ", new String[] {"1.23456789E9"}); + testSimpleEval("select round(1234567890,1) ", new String[]{"1.23456789E9"} ); + testSimpleEval("select round(1234567890,2) ", new String[] {"1.23456789E9"} ); testSimpleEval("select round(1.2345678901234567,13) ", new String[] {"1.2345678901235"} ); testSimpleEval("select round(1234567890.1234567,3) ", new String[] {"1234567890.123"} ); testSimpleEval("select round(1234567890.1234567,5) ", new String[] {"1234567890.12346"} ); //testSimpleEval("select round(1234567890.1234567890,7) ", new String[] {"1234567890.1234568"}); + testSimpleEval("select round(1234567890.1234567,3) ", new String[]{"1.234567890123E9"}); + testSimpleEval("select round(1234567890.1234567,5) ", new String[]{"1.23456789012346E9"}); +//testSimpleEval("select round(1234567890.1234567890,7) ", new String[]{"1234567890.1234568"} ); End diff – I'll remove.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user babokim commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15564097

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java —
        @@ -70,23 +63,21 @@ public Datum eval(Tuple params)

        { return NullDatum.get(); }
        • if (numberFormat == null || !formatConstant) { - numberFormat = NumberFormat.getInstance(); - numberFormat.setGroupingUsed(false); - numberFormat.setMaximumFractionDigits(roundDatum.asInt4()); - }

          -
          double value = valueDatum.asFloat8();

        • int roundPnt = roundDatum.asInt4();
        • double roundNum;
          + int rountPoint = roundDatum.asInt4();
        • if (value > 0) {
        • roundNum = (long)(value * Math.pow(10, roundPnt) + 0.5d) / Math.pow(10, roundPnt);
          + if (Double.isNaN(value)) { + throw new InvalidOperationException("value is not a number"); }
        • else {
        • roundNum = (long)(value * Math.pow(10, roundPnt) - 0.5d) / Math.pow(10, roundPnt);
          +
          + if (Double.isInfinite(value)) {
          + throw new InvalidOperationException("/ by zero");
            • End diff –

        Ok. I'll change "value is infinite."

        Show
        githubbot ASF GitHub Bot added a comment - Github user babokim commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15564097 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java — @@ -70,23 +63,21 @@ public Datum eval(Tuple params) { return NullDatum.get(); } if (numberFormat == null || !formatConstant) { - numberFormat = NumberFormat.getInstance(); - numberFormat.setGroupingUsed(false); - numberFormat.setMaximumFractionDigits(roundDatum.asInt4()); - } - double value = valueDatum.asFloat8(); int roundPnt = roundDatum.asInt4(); double roundNum; + int rountPoint = roundDatum.asInt4(); if (value > 0) { roundNum = (long)(value * Math.pow(10, roundPnt) + 0.5d) / Math.pow(10, roundPnt); + if (Double.isNaN(value)) { + throw new InvalidOperationException("value is not a number"); } else { roundNum = (long)(value * Math.pow(10, roundPnt) - 0.5d) / Math.pow(10, roundPnt); + + if (Double.isInfinite(value)) { + throw new InvalidOperationException("/ by zero"); End diff – Ok. I'll change "value is infinite."
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/96#issuecomment-50293741

        Hi @babokim, this patch looks good to me.
        I left some trivial comments.
        Thanks.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/96#issuecomment-50293741 Hi @babokim, this patch looks good to me. I left some trivial comments. Thanks.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15444292

        — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java —
        @@ -470,4 +470,40 @@ public final void testNowInMultipleTasks() throws Exception

        { executeString("DROP TABLE table11 PURGE"); }

        }
        +
        + @Test
        + public void testCaseWhenRound() throws Exception {
        — End diff –

        Would you mind improving this test to use executeFile() instead of executeString()?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15444292 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestSelectQuery.java — @@ -470,4 +470,40 @@ public final void testNowInMultipleTasks() throws Exception { executeString("DROP TABLE table11 PURGE"); } } + + @Test + public void testCaseWhenRound() throws Exception { — End diff – Would you mind improving this test to use executeFile() instead of executeString()?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15444279

        — Diff: tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java —
        @@ -428,19 +430,27 @@ public void testPi() throws IOException {

        @Test
        public void testRoundWithSpecifiedPrecision() throws IOException {
        + // divide zero
        + try {
        + testSimpleEval("select round(10.0/0.0,2) ", new String[]

        {""}

        );
        + fail("10.0/0 should throw InvalidOperationException");
        + } catch (InvalidOperationException e)

        { + //success + }

        +
        testSimpleEval("select round(42.4382,2) ", new String[]

        {"42.44"}

        );
        testSimpleEval("select round(-42.4382,2) ", new String[]

        {"-42.44"}

        );

        • testSimpleEval("select round(-425,2) ", new String[] {"-425"}

          );

        • testSimpleEval("select round(425,2) ", new String[] {"425"}

          );
          + testSimpleEval("select round(-425,2) ", new String[]

          {"-425.0"}

          );
          + testSimpleEval("select round(425,2) ", new String[]

          {"425.0"}

          );

        • testSimpleEval("select round(1234567890,0) ", new String[] {"1234567890"});
          - testSimpleEval("select round(1234567890,1) ", new String[]{"1234567890"}

          );

        • testSimpleEval("select round(1234567890,2) ", new String[] {"1234567890"}

          );
          + testSimpleEval("select round(1234567890,0) ", new String[]

          {"1.23456789E9"});
          + testSimpleEval("select round(1234567890,1) ", new String[]{"1.23456789E9"}

          );
          + testSimpleEval("select round(1234567890,2) ", new String[]

          {"1.23456789E9"}

          );

        testSimpleEval("select round(1.2345678901234567,13) ", new String[]

        {"1.2345678901235"}

        );

        • testSimpleEval("select round(1234567890.1234567,3) ", new String[] {"1234567890.123"}

          );

        • testSimpleEval("select round(1234567890.1234567,5) ", new String[] {"1234567890.12346"}

          );

        • //testSimpleEval("select round(1234567890.1234567890,7) ", new String[] {"1234567890.1234568"});
          + testSimpleEval("select round(1234567890.1234567,3) ", new String[]{"1.234567890123E9"});
          + testSimpleEval("select round(1234567890.1234567,5) ", new String[]{"1.23456789012346E9"});
          +//testSimpleEval("select round(1234567890.1234567890,7) ", new String[]{"1234567890.1234568"}

          );

            • End diff –

        Please remove commented out line.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15444279 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java — @@ -428,19 +430,27 @@ public void testPi() throws IOException { @Test public void testRoundWithSpecifiedPrecision() throws IOException { + // divide zero + try { + testSimpleEval("select round(10.0/0.0,2) ", new String[] {""} ); + fail("10.0/0 should throw InvalidOperationException"); + } catch (InvalidOperationException e) { + //success + } + testSimpleEval("select round(42.4382,2) ", new String[] {"42.44"} ); testSimpleEval("select round(-42.4382,2) ", new String[] {"-42.44"} ); testSimpleEval("select round(-425,2) ", new String[] {"-425"} ); testSimpleEval("select round(425,2) ", new String[] {"425"} ); + testSimpleEval("select round(-425,2) ", new String[] {"-425.0"} ); + testSimpleEval("select round(425,2) ", new String[] {"425.0"} ); testSimpleEval("select round(1234567890,0) ", new String[] {"1234567890"}); - testSimpleEval("select round(1234567890,1) ", new String[]{"1234567890"} ); testSimpleEval("select round(1234567890,2) ", new String[] {"1234567890"} ); + testSimpleEval("select round(1234567890,0) ", new String[] {"1.23456789E9"}); + testSimpleEval("select round(1234567890,1) ", new String[]{"1.23456789E9"} ); + testSimpleEval("select round(1234567890,2) ", new String[] {"1.23456789E9"} ); testSimpleEval("select round(1.2345678901234567,13) ", new String[] {"1.2345678901235"} ); testSimpleEval("select round(1234567890.1234567,3) ", new String[] {"1234567890.123"} ); testSimpleEval("select round(1234567890.1234567,5) ", new String[] {"1234567890.12346"} ); //testSimpleEval("select round(1234567890.1234567890,7) ", new String[] {"1234567890.1234568"}); + testSimpleEval("select round(1234567890.1234567,3) ", new String[]{"1.234567890123E9"}); + testSimpleEval("select round(1234567890.1234567,5) ", new String[]{"1.23456789012346E9"}); +//testSimpleEval("select round(1234567890.1234567890,7) ", new String[]{"1234567890.1234568"} ); End diff – Please remove commented out line.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/96#discussion_r15444253

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java —
        @@ -70,23 +63,21 @@ public Datum eval(Tuple params)

        { return NullDatum.get(); }
        • if (numberFormat == null || !formatConstant) { - numberFormat = NumberFormat.getInstance(); - numberFormat.setGroupingUsed(false); - numberFormat.setMaximumFractionDigits(roundDatum.asInt4()); - }

          -
          double value = valueDatum.asFloat8();

        • int roundPnt = roundDatum.asInt4();
        • double roundNum;
          + int rountPoint = roundDatum.asInt4();
        • if (value > 0) {
        • roundNum = (long)(value * Math.pow(10, roundPnt) + 0.5d) / Math.pow(10, roundPnt);
          + if (Double.isNaN(value)) { + throw new InvalidOperationException("value is not a number"); }
        • else {
        • roundNum = (long)(value * Math.pow(10, roundPnt) - 0.5d) / Math.pow(10, roundPnt);
          +
          + if (Double.isInfinite(value)) {
          + throw new InvalidOperationException("/ by zero");
            • End diff –

        The exception message should be fixed because this is not divide operation.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/96#discussion_r15444253 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/math/RoundFloat8.java — @@ -70,23 +63,21 @@ public Datum eval(Tuple params) { return NullDatum.get(); } if (numberFormat == null || !formatConstant) { - numberFormat = NumberFormat.getInstance(); - numberFormat.setGroupingUsed(false); - numberFormat.setMaximumFractionDigits(roundDatum.asInt4()); - } - double value = valueDatum.asFloat8(); int roundPnt = roundDatum.asInt4(); double roundNum; + int rountPoint = roundDatum.asInt4(); if (value > 0) { roundNum = (long)(value * Math.pow(10, roundPnt) + 0.5d) / Math.pow(10, roundPnt); + if (Double.isNaN(value)) { + throw new InvalidOperationException("value is not a number"); } else { roundNum = (long)(value * Math.pow(10, roundPnt) - 0.5d) / Math.pow(10, roundPnt); + + if (Double.isInfinite(value)) { + throw new InvalidOperationException("/ by zero"); End diff – The exception message should be fixed because this is not divide operation.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user babokim opened a pull request:

        https://github.com/apache/tajo/pull/96

        TAJO-978: RoundFloat8 should return Float8Datum type.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/babokim/tajo TAJO-978

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tajo/pull/96.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #96


        commit 4b1e03cbbe02698e84270d0f27f3fb9921c3079e
        Author: 김형준 <babokim@babokim-mbp.server.gruter.com>
        Date: 2014-07-25T12:37:35Z

        TAJO-978: RoundFloat8 should return Float8Datum type.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user babokim opened a pull request: https://github.com/apache/tajo/pull/96 TAJO-978 : RoundFloat8 should return Float8Datum type. You can merge this pull request into a Git repository by running: $ git pull https://github.com/babokim/tajo TAJO-978 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/96.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #96 commit 4b1e03cbbe02698e84270d0f27f3fb9921c3079e Author: 김형준 <babokim@babokim-mbp.server.gruter.com> Date: 2014-07-25T12:37:35Z TAJO-978 : RoundFloat8 should return Float8Datum type.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development