You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by sirpkt <gi...@git.apache.org> on 2014/12/22 06:33:36 UTC
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
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 <si...@apache.org>
Date: 2014-12-22T05:26:04Z
TAJO-1265: min(), max() does not handle null properly
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by sirpkt <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/315
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by sirpkt <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by sirpkt <gi...@git.apache.org>.
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().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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'.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by sirpkt <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by sirpkt <gi...@git.apache.org>.
Github user sirpkt commented on the pull request:
https://github.com/apache/tajo/pull/315#issuecomment-69148873
I just committed the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1265: min(), max() does not handle null pr...
Posted by hyunsik <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---