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

min(), max() does not handle null properly

    Details

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

      Description

      In other DBs like oracle and postgreSQL,
      null is excluded from calculation of min() and max() value until there is no non-null value in the given column, in which case, it returns null.
      However, current implementation of Tajo considers null as
      0 for int or long column, 0.0 for float or double column, or "" for text column.
      It needs to handle null value separately from non-null values in min() and max() calculation.

        Activity

        Hide
        hudson Hudson added a comment -

        ABORTED: Integrated in Tajo-master-CODEGEN-build #179 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/179/)
        TAJO-1265: min(), max() does not handle null properly. (Keuntae Park) (sirpkt: rev a1e03289b35d34115a449ab7d81b946f69400210)

        • tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable4.result
        • tajo-core/src/test/java/org/apache/tajo/engine/function/TestBuiltinFunctions.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData8.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumFloat.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData4.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinLong.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxDouble.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/DistinctGroupbySortAggregationExec.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxFloat.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable4.result
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData7.result
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData3.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxLong.java
        • tajo-core/src/test/resources/queries/TestBuiltinFunctions/testAvgLongOverflow.sql
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Min.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxInt.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgInt.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxString.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java
        • tajo-core/src/test/java/org/apache/tajo/engine/query/TestGroupByQuery.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinString.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData6.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Max.java
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable2.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinDouble.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinFloat.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumInt.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgFloat.java
        • tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable2.result
        • tajo-core/src/test/resources/results/TestBuiltinFunctions/testAvgLongOverflow.result
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable5.result
        • CHANGES
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinInt.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData2.result
        Show
        hudson Hudson added a comment - ABORTED: Integrated in Tajo-master-CODEGEN-build #179 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/179/ ) TAJO-1265 : min(), max() does not handle null properly. (Keuntae Park) (sirpkt: rev a1e03289b35d34115a449ab7d81b946f69400210) tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable4.result tajo-core/src/test/java/org/apache/tajo/engine/function/TestBuiltinFunctions.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData8.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumFloat.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData4.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinLong.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxDouble.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/DistinctGroupbySortAggregationExec.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxFloat.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable4.result tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData7.result tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData3.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxLong.java tajo-core/src/test/resources/queries/TestBuiltinFunctions/testAvgLongOverflow.sql tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Min.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxInt.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgInt.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxString.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java tajo-core/src/test/java/org/apache/tajo/engine/query/TestGroupByQuery.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinString.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData6.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Max.java tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable2.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinDouble.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinFloat.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumInt.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgFloat.java tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable2.result tajo-core/src/test/resources/results/TestBuiltinFunctions/testAvgLongOverflow.result tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable5.result CHANGES tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinInt.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData2.result
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #540 (See https://builds.apache.org/job/Tajo-master-build/540/)
        TAJO-1265: min(), max() does not handle null properly. (Keuntae Park) (sirpkt: rev a1e03289b35d34115a449ab7d81b946f69400210)

        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData7.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxDouble.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxLong.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumInt.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData8.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinDouble.java
        • tajo-core/src/test/java/org/apache/tajo/engine/function/TestBuiltinFunctions.java
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable5.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable2.result
        • tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable4.result
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData4.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgFloat.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgInt.java
        • tajo-core/src/test/resources/queries/TestBuiltinFunctions/testAvgLongOverflow.sql
        • tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable4.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Max.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData2.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxFloat.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinString.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumFloat.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData6.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java
        • tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable2.result
        • tajo-core/src/test/resources/results/TestBuiltinFunctions/testAvgLongOverflow.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Min.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinInt.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxInt.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinLong.java
        • CHANGES
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxString.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/DistinctGroupbySortAggregationExec.java
        • tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData3.result
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java
        • tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinFloat.java
        • tajo-core/src/test/java/org/apache/tajo/engine/query/TestGroupByQuery.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #540 (See https://builds.apache.org/job/Tajo-master-build/540/ ) TAJO-1265 : min(), max() does not handle null properly. (Keuntae Park) (sirpkt: rev a1e03289b35d34115a449ab7d81b946f69400210) tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData7.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxDouble.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxLong.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumInt.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData8.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinDouble.java tajo-core/src/test/java/org/apache/tajo/engine/function/TestBuiltinFunctions.java tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable5.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable2.result tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable4.result tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData4.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgFloat.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgInt.java tajo-core/src/test/resources/queries/TestBuiltinFunctions/testAvgLongOverflow.sql tajo-core/src/test/resources/results/TestJoinQuery/testLeftOuterJoinWithEmptyTable4.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Max.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData2.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxFloat.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinString.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumFloat.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData6.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java tajo-core/src/test/resources/results/TestJoinBroadcast/testLeftOuterJoinWithEmptyTable2.result tajo-core/src/test/resources/results/TestBuiltinFunctions/testAvgLongOverflow.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/Min.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinInt.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxInt.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinLong.java CHANGES tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MaxString.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/DistinctGroupbySortAggregationExec.java tajo-core/src/test/resources/results/TestGroupByQuery/testGroupByWithNullData3.result tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/MinFloat.java tajo-core/src/test/java/org/apache/tajo/engine/query/TestGroupByQuery.java
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sirpkt commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-69148873

        I just committed the patch.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-69148873 I just committed the patch.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

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

        Github user sirpkt commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-69125520

        Thank you for the review, @hyunsik.
        I removed unused imports.
        I checked 'mvn clean install', however, I'll commit after Travis CI build is done without problem.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-69125520 Thank you for the review, @hyunsik. I removed unused imports. I checked 'mvn clean install', however, I'll commit after Travis CI build is done without problem.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68902301

        Hi @sirpkt,

        I leave some trivial comments. Because they are trivial, you can commit after you remove them.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68902301 Hi @sirpkt, I leave some trivial comments. Because they are trivial, you can commit after you remove them.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68901952

        +1
        Thank you for finding these bugs. It seems to correct wrong null handling behavior of aggregation operators.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68901952 +1 Thank you for finding these bugs. It seems to correct wrong null handling behavior of aggregation operators.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/315#discussion_r22537236

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java —
        @@ -18,6 +18,7 @@

        package org.apache.tajo.engine.function.builtin;

        +import org.apache.commons.math.stat.descriptive.summary.Sum;
        — End diff –

        unused import

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/315#discussion_r22537236 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumLong.java — @@ -18,6 +18,7 @@ package org.apache.tajo.engine.function.builtin; +import org.apache.commons.math.stat.descriptive.summary.Sum; — End diff – unused import
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/315#discussion_r22537205

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java —
        @@ -25,6 +25,7 @@
        import org.apache.tajo.datum.Datum;
        import org.apache.tajo.datum.DatumFactory;
        import org.apache.tajo.datum.Float8Datum;
        — End diff –

        unused import

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/315#discussion_r22537205 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/SumDouble.java — @@ -25,6 +25,7 @@ import org.apache.tajo.datum.Datum; import org.apache.tajo.datum.DatumFactory; import org.apache.tajo.datum.Float8Datum; — End diff – unused import
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/315#discussion_r22537086

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java —
        @@ -18,6 +18,7 @@

        package org.apache.tajo.engine.function.builtin;

        +import org.apache.commons.lang.ObjectUtils;
        — End diff –

        unused import

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/315#discussion_r22537086 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgLong.java — @@ -18,6 +18,7 @@ package org.apache.tajo.engine.function.builtin; +import org.apache.commons.lang.ObjectUtils; — End diff – unused import
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/315#discussion_r22537069

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java —
        @@ -18,6 +18,7 @@

        package org.apache.tajo.engine.function.builtin;

        +import org.apache.commons.lang.ObjectUtils;
        — End diff –

        Unused import

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/315#discussion_r22537069 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/builtin/AvgDouble.java — @@ -18,6 +18,7 @@ package org.apache.tajo.engine.function.builtin; +import org.apache.commons.lang.ObjectUtils; — End diff – Unused import
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68782021

        The patch looks good to me. It corrects the wrong implementation. There are some trivial things that I need to check. I'll finish the review by tomorrow.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68782021 The patch looks good to me. It corrects the wrong implementation. There are some trivial things that I need to check. I'll finish the review by tomorrow.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68698532

        I'll review it tonight. Thank you for your contribution.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68698532 I'll review it tonight. Thank you for your contribution.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sirpkt commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68671030

        I also modified sum() and avg() to handle null properly.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68671030 I also modified sum() and avg() to handle null properly.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sirpkt commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-68024356

        I updated the patch as followings

        • null handling bug fix in getPartialResult() of Min and Max
        • getEmptyTuple() in DistinctGroupbySortAggregationExec.java is changed to make Tuple according to the type of aggregation functions: currently, null datum for min and max and 0 for other types.
        • Test result is fixed as correct value

        mvn clean install passed.

        sum() and avg() also need to be changed to handle null differently from non-null values.
        I'll make another issue about sum() and avg().

        Show
        githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-68024356 I updated the patch as followings null handling bug fix in getPartialResult() of Min and Max getEmptyTuple() in DistinctGroupbySortAggregationExec.java is changed to make Tuple according to the type of aggregation functions: currently, null datum for min and max and 0 for other types. Test result is fixed as correct value mvn clean install passed. sum() and avg() also need to be changed to handle null differently from non-null values. I'll make another issue about sum() and avg().
        Hide
        sirpkt Keuntae Park added a comment -

        It seems that it is not a simple task to make a patch especially when it deals empty table cases.

        According ANSI SQL standard, for empty table,
        min(), max(), sum(), avg() return null, however, count() returns 0.

        Currently, getEmptyTuple() in DistinctGroupbySortAggregationExec.java makes all the aggregation result for empty table as scalar value.
        It should be changed to return null for min, max, sum, avg() and 0 for count().
        And sum, avg() code should also be changed to correctly handle null value.

        As I'm not familiar with this part of code, it will need more time to make a patch.

        Please let me know if I mistake on some point or having any comments.

        Show
        sirpkt Keuntae Park added a comment - It seems that it is not a simple task to make a patch especially when it deals empty table cases. According ANSI SQL standard, for empty table, min(), max(), sum(), avg() return null, however, count() returns 0. Currently, getEmptyTuple() in DistinctGroupbySortAggregationExec.java makes all the aggregation result for empty table as scalar value. It should be changed to return null for min, max, sum, avg() and 0 for count(). And sum, avg() code should also be changed to correctly handle null value. As I'm not familiar with this part of code, it will need more time to make a patch. Please let me know if I mistake on some point or having any comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sirpkt commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-67810799

        @hyunsik Sorry for the incomplete patch.
        There are many test cases related with max(), min() and I missed the testing for all those cases.
        I apologize for stealing your time by faulty patch.
        I'll check and update the patch.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-67810799 @hyunsik Sorry for the incomplete patch. There are many test cases related with max(), min() and I missed the testing for all those cases. I apologize for stealing your time by faulty patch. I'll check and update the patch.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/315#issuecomment-67808038

        This patch seems to not pass 'mvn clean install'.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/315#issuecomment-67808038 This patch seems to not pass 'mvn clean install'.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user sirpkt opened a pull request:

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

        TAJO-1265: min(), max() does not handle null properly

        min() and max() handle null value separately from non-null values.
        It computes min() or max() among non-null values first.
        If no non-null value exists, they return null value.
        Implementation style of min() and max() is also changed that
        there are abstract classes of Min and Max common for all the data types
        and only type specific methods are implemented for each type.

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

        $ git pull https://github.com/sirpkt/tajo TAJO-1265

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

        https://github.com/apache/tajo/pull/315.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 #315


        commit 46c5dd47deac696b19536ee5bb78667006bad8a5
        Author: sirpkt <sirpkt@apache.org>
        Date: 2014-12-22T05:26:04Z

        TAJO-1265: min(), max() does not handle null properly


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user sirpkt opened a pull request: https://github.com/apache/tajo/pull/315 TAJO-1265 : min(), max() does not handle null properly min() and max() handle null value separately from non-null values. It computes min() or max() among non-null values first. If no non-null value exists, they return null value. Implementation style of min() and max() is also changed that there are abstract classes of Min and Max common for all the data types and only type specific methods are implemented for each type. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sirpkt/tajo TAJO-1265 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/315.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 #315 commit 46c5dd47deac696b19536ee5bb78667006bad8a5 Author: sirpkt <sirpkt@apache.org> Date: 2014-12-22T05:26:04Z TAJO-1265 : min(), max() does not handle null properly

          People

          • Assignee:
            sirpkt Keuntae Park
            Reporter:
            sirpkt Keuntae Park
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development