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

Tajo cannot recognize negative numeric expressions

    Details

      Description

      The parser cannot deal with negative numeric values. You can reproduce the problem as follows:

      select -1;
      

      We have to fix it.

      1. TAJO-344.patch
        31 kB
        Hyunsik Choi

        Issue Links

          Activity

          Hide
          hyunsik Hyunsik Choi added a comment -

          I've attached the patch. This patch modifies SQL parser, algebraic expression, logical plan in order to support sign numeric values. In addition, this patch removes comments of unit tests including minus constant values.

          I verified 'mvn clean install'.

          Show
          hyunsik Hyunsik Choi added a comment - I've attached the patch. This patch modifies SQL parser, algebraic expression, logical plan in order to support sign numeric values. In addition, this patch removes comments of unit tests including minus constant values. I verified 'mvn clean install'.
          Hide
          jihoonson Jihoon Son added a comment -

          I'm reviewing the patch.

          Show
          jihoonson Jihoon Son added a comment - I'm reviewing the patch.
          Hide
          jihoonson Jihoon Son added a comment -

          Overall, this patch looks good to me.
          But, I have a question for the round function.
          You use Math.ceil() and Math.floor() for negative and positive values, respectively.
          However, it looks to return the same results with Math.round() as follows.

          Math.round(10.3d) => 10
          Math.round(10.7d) => 11
          Math.round(-10.7d) => -11
          Math.round(-10.3d) => -10
          
          Tajo.round(10.3d) => 10
          Tajo.round(10.7d) => 11
          Tajo.round(-10.7d) => -11
          Tajo.round(-10.3d) => -10
          (The Tajo.round() function means that the Round() function of Tajo.)
          

          Do you have any reasons to use the above two functions instead of just using Math.round()?

          Also, the patch causes a Hunk while applying it.

          Show
          jihoonson Jihoon Son added a comment - Overall, this patch looks good to me. But, I have a question for the round function. You use Math.ceil() and Math.floor() for negative and positive values, respectively. However, it looks to return the same results with Math.round() as follows. Math.round(10.3d) => 10 Math.round(10.7d) => 11 Math.round(-10.7d) => -11 Math.round(-10.3d) => -10 Tajo.round(10.3d) => 10 Tajo.round(10.7d) => 11 Tajo.round(-10.7d) => -11 Tajo.round(-10.3d) => -10 (The Tajo.round() function means that the Round() function of Tajo.) Do you have any reasons to use the above two functions instead of just using Math.round()? Also, the patch causes a Hunk while applying it.
          Hide
          hyunsik Hyunsik Choi added a comment - - edited

          Thank you for your detailed review.

          The result of Math.round may be different from other programming languages. This is because there are various round half-down/up manners [1] and Math.round uses some different round manner. So, we can get a mismatch as follows:

          Math.round
          Math.round(-5.5) -> -5
          
          PostgreSQL
          hyunsik=> select round(-5.5);
           round 
          -------
              -6
          (1 row)
          
          MySQL
          MariaDB [(none)]> select round(-5.5);
          +-------------+
          | round(-5.5) |
          +-------------+
          |          -6 |
          +-------------+
          1 row in set (0.00 sec)
          

          Especially, we could face the result mismatch problem when we use a vectorized engine based on C++. This patch also contains the workaround code. I missed the detail comment for this reason. Since this workaround code may make other guys confuse, I'll add some notes on Round class.

          In addition, in my opinion, succeeded hunks are not problem because it just means somewhat difference of diff lines but success. If we have to rebase all patches that cause even success hunk, contributors will be exhausted due to too frequent rebase.

          [1] https://en.wikipedia.org/wiki/Rounding#Round_half_up

          Show
          hyunsik Hyunsik Choi added a comment - - edited Thank you for your detailed review. The result of Math.round may be different from other programming languages. This is because there are various round half-down/up manners [1] and Math.round uses some different round manner. So, we can get a mismatch as follows: Math.round Math .round(-5.5) -> -5 PostgreSQL hyunsik=> select round(-5.5); round ------- -6 (1 row) MySQL MariaDB [(none)]> select round(-5.5); +-------------+ | round(-5.5) | +-------------+ | -6 | +-------------+ 1 row in set (0.00 sec) Especially, we could face the result mismatch problem when we use a vectorized engine based on C++. This patch also contains the workaround code. I missed the detail comment for this reason. Since this workaround code may make other guys confuse, I'll add some notes on Round class. In addition, in my opinion, succeeded hunks are not problem because it just means somewhat difference of diff lines but success. If we have to rebase all patches that cause even success hunk, contributors will be exhausted due to too frequent rebase. [1] https://en.wikipedia.org/wiki/Rounding#Round_half_up
          Hide
          jihoonson Jihoon Son added a comment -

          Thanks for your comment.
          I understand your point.
          +1 for this patch.

          Show
          jihoonson Jihoon Son added a comment - Thanks for your comment. I understand your point. +1 for this patch.
          Hide
          hyunsik Hyunsik Choi added a comment -

          Thank you for the review. I added some notes in Round.java, and committed the patch to master branch.

          Show
          hyunsik Hyunsik Choi added a comment - Thank you for the review. I added some notes in Round.java, and committed the patch to master branch.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Tajo-trunk-postcommit #598 (See https://builds.apache.org/job/Tajo-trunk-postcommit/598/)
          TAJO-344: Tajo cannot recognize negative numeric expressions. (hyunsik) (hyunsik: https://git-wip-us.apache.org/repos/asf?p=incubator-tajo.git&a=commit&h=17d9cd7357d9e4e079a61acbaa755206c5f019db)

          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/math/Round.java
          • tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/EvalType.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Float8Datum.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/OpType.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/SignedExpr.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int4Datum.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Float4Datum.java
          • CHANGES.txt
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/EvalNode.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int2Datum.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/LiteralValue.java
          • tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
          • tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestStringOperatorsAndFunctions.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/SignedEval.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
          • tajo-common/src/main/java/org/apache/tajo/datum/NumericDatum.java
          • tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/string/Substr.java
          • tajo-common/src/main/java/org/apache/tajo/datum/Int8Datum.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Tajo-trunk-postcommit #598 (See https://builds.apache.org/job/Tajo-trunk-postcommit/598/ ) TAJO-344 : Tajo cannot recognize negative numeric expressions. (hyunsik) (hyunsik: https://git-wip-us.apache.org/repos/asf?p=incubator-tajo.git&a=commit&h=17d9cd7357d9e4e079a61acbaa755206c5f019db ) tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/math/Round.java tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestMathFunctions.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/EvalType.java tajo-common/src/main/java/org/apache/tajo/datum/Float8Datum.java tajo-algebra/src/main/java/org/apache/tajo/algebra/OpType.java tajo-algebra/src/main/java/org/apache/tajo/algebra/SignedExpr.java tajo-common/src/main/java/org/apache/tajo/datum/Int4Datum.java tajo-common/src/main/java/org/apache/tajo/datum/Float4Datum.java CHANGES.txt tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/EvalNode.java tajo-common/src/main/java/org/apache/tajo/datum/Int2Datum.java tajo-algebra/src/main/java/org/apache/tajo/algebra/LiteralValue.java tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/function/TestStringOperatorsAndFunctions.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/eval/SignedEval.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-common/src/main/java/org/apache/tajo/datum/NumericDatum.java tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/string/Substr.java tajo-common/src/main/java/org/apache/tajo/datum/Int8Datum.java

            People

            • Assignee:
              hyunsik Hyunsik Choi
              Reporter:
              hyunsik Hyunsik Choi
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development