You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by KurtYoung <gi...@git.apache.org> on 2017/07/12 06:07:21 UTC
[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
GitHub user KurtYoung opened a pull request:
https://github.com/apache/flink/pull/4305
[FLINK-7161] fix misusage of Float.MIN_VALUE and Double.MIN_VALUE
I already checked all places using Float.MIN_VALUE and Double.MIN_VALUE and fixed all misusage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/KurtYoung/flink flink-7161
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4305.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 #4305
----
commit 5c7a2fd4c97eaedba87b0042e337d94f315b4191
Author: Kurt Young <yk...@gmail.com>
Date: 2017-07-12T06:02:58Z
[FLINK-7161] fix misusage of Float.MIN_VALUE and Double.MIN_VALUE
----
---
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] flink issue #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and Double.M...
Posted by KurtYoung <gi...@git.apache.org>.
Github user KurtYoung commented on the issue:
https://github.com/apache/flink/pull/4305
Hi @greghogan , thanks for the review, your comments have been addressed.
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/4305#discussion_r127109325
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -843,11 +843,14 @@ private float convertToFloat(Object o, float defaultValue) {
}
else if (o.getClass() == Double.class) {
double value = ((Double) o);
- if (value <= Float.MAX_VALUE && value >= Float.MIN_VALUE) {
- return (float) value;
- } else {
+ if (value < -Float.MAX_VALUE
--- End diff --
I stand corrected. Why not use three checks instead of four (five comparisons instead of six) with the additional equality check with `0`?
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/4305#discussion_r126946827
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -843,11 +843,14 @@ private float convertToFloat(Object o, float defaultValue) {
}
else if (o.getClass() == Double.class) {
double value = ((Double) o);
- if (value <= Float.MAX_VALUE && value >= Float.MIN_VALUE) {
- return (float) value;
- } else {
+ if (value < -Float.MAX_VALUE
--- End diff --
The current checks around `0` can be combined. Can we simplify this to two ranges, `-max` to `-min` and `min` to `max` (all inclusive), reverting the order of the if-else?
---
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] flink issue #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and Double.M...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4305
Good catches, +1 to merge
---
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] flink issue #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and Double.M...
Posted by zhangminglei <gi...@git.apache.org>.
Github user zhangminglei commented on the issue:
https://github.com/apache/flink/pull/4305
Looks good, as ```Double.MIN_VALUE``` always return a positive number.
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/4305#discussion_r126921674
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -843,7 +843,7 @@ private float convertToFloat(Object o, float defaultValue) {
}
else if (o.getClass() == Double.class) {
double value = ((Double) o);
- if (value <= Float.MAX_VALUE && value >= Float.MIN_VALUE) {
+ if (value <= Float.MAX_VALUE && value >= Float.NEGATIVE_INFINITY) {
--- End diff --
The original looks to be the correct test for underflow but should also test the negative range.
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by KurtYoung <gi...@git.apache.org>.
Github user KurtYoung commented on a diff in the pull request:
https://github.com/apache/flink/pull/4305#discussion_r127107209
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -843,11 +843,14 @@ private float convertToFloat(Object o, float defaultValue) {
}
else if (o.getClass() == Double.class) {
double value = ((Double) o);
- if (value <= Float.MAX_VALUE && value >= Float.MIN_VALUE) {
- return (float) value;
- } else {
+ if (value < -Float.MAX_VALUE
--- End diff --
I'm afraid in that case, 0 can not be treated right, so i add a test case to check.
---
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] flink issue #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and Double.M...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4305
+1
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by KurtYoung <gi...@git.apache.org>.
Github user KurtYoung commented on a diff in the pull request:
https://github.com/apache/flink/pull/4305#discussion_r127113118
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/Configuration.java ---
@@ -843,11 +843,14 @@ private float convertToFloat(Object o, float defaultValue) {
}
else if (o.getClass() == Double.class) {
double value = ((Double) o);
- if (value <= Float.MAX_VALUE && value >= Float.MIN_VALUE) {
- return (float) value;
- } else {
+ if (value < -Float.MAX_VALUE
--- End diff --
I'm ok with both, will write it in your way.
---
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] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4305
---
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.
---