You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sahitya-pavurala <gi...@git.apache.org> on 2015/06/17 20:48:38 UTC
[GitHub] flink pull request: check in if clause in LongValue.java
GitHub user sahitya-pavurala opened a pull request:
https://github.com/apache/flink/pull/849
check in if clause in LongValue.java
Should be logical(&&) and not bit wise(&)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sahitya-pavurala/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/849.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 #849
----
commit 0232c3bd623401603c914140f8cab882e8f6c2a6
Author: sahitya pavurala <sp...@nyu.edu>
Date: 2015-06-17T18:47:42Z
check in if clause
Should be logical(&&) and not bit wise(&)
----
---
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: check in if clause in LongValue.java
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-112940775
It looks like I deleted my local repository accidentally. Let me know if I have to make this change again.
---
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: check in if clause in LongValue.java
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113298367
I think that the correct thing is to drop the check for not null, because the `instanceof` keyword checks that the argument is not null already. The current code hence checks twice that the value is not null.
---
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: check in if clause in LongValue.java
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113079519
Thanks für the PR.
As long as you keep the PR branch in your GH repository, things are fine.
---
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: check in if clause in LongValue.java
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113110114
okay thanks , so after you merge the code , there are few more things to be changed in those classes , will make the changes shortly.
---
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: check in if clause in LongValue.java
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113270413
Okay, 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] flink pull request: check in if clause in LongValue.java
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113270184
Hi @sahitya-pavurala , if the PR is merged we will automatically close the PR for you.
---
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: check in if clause in LongValue.java
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113120481
Since both are booleans, this check is equivalent to `&&` w/o the short-circuit evaluation. So good 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 pull request: check in if clause in LongValue.java
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/849
---
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: check in if clause in LongValue.java
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113299510
I think that the instanceOf should be changed to getClass in the first place. getClass makes more sense since its a equals method and we want both the types to be identical(not allow sub types)
---
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: check in if clause in LongValue.java
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/849#issuecomment-113232609
I am relatively new to github, i cant close the pr until you merge the code right?
---
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.
---