[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[asfgit]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[KurtYoung]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[greghogan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[KurtYoung]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[greghogan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[greghogan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #4305: [FLINK-7161] fix misusage of Float.MIN_VALUE and D...

[KurtYoung]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---