You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by babokim <gi...@git.apache.org> on 2014/07/25 14:43:18 UTC
[GitHub] tajo pull request: TAJO-978: RoundFloat8 should return Float8Datum...
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: 김형준 <ba...@babokim-mbp.server.gruter.com>
Date: 2014-07-25T12:37:35Z
TAJO-978: RoundFloat8 should return Float8Datum type.
----
---
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-978: RoundFloat8 should return Float8Datum...
Posted by hyunsik <gi...@git.apache.org>.
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!
---
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-978: RoundFloat8 should return Float8Datum...
Posted by babokim <gi...@git.apache.org>.
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.
---
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-978: RoundFloat8 should return Float8Datum...
Posted by babokim <gi...@git.apache.org>.
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.
---
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-978: RoundFloat8 should return Float8Datum...
Posted by babokim <gi...@git.apache.org>.
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."
---
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-978: RoundFloat8 should return Float8Datum...
Posted by jihoonson <gi...@git.apache.org>.
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.
---
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-978: RoundFloat8 should return Float8Datum...
Posted by jihoonson <gi...@git.apache.org>.
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.
---
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-978: RoundFloat8 should return Float8Datum...
Posted by jihoonson <gi...@git.apache.org>.
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()?
---
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-978: RoundFloat8 should return Float8Datum...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/96
---
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-978: RoundFloat8 should return Float8Datum...
Posted by babokim <gi...@git.apache.org>.
Github user babokim commented on the pull request:
https://github.com/apache/tajo/pull/96#issuecomment-50566282
I applied your comments. Please review. Thanks.
---
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-978: RoundFloat8 should return Float8Datum...
Posted by jihoonson <gi...@git.apache.org>.
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.
---
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.
---