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 23:17:36 UTC
[GitHub] flink pull request: Modified the equals method in all the Value ty...
GitHub user sahitya-pavurala opened a pull request:
https://github.com/apache/flink/pull/850
Modified the equals method in all the Value type classes
Added check for null in all the equals methods in all the Value type classes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sahitya-pavurala/flink myBranch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/850.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 #850
----
commit 03e703382b2b68a03275825e16bfcbf302bac161
Author: sahitya-pavurala <sp...@nyu.edu>
Date: 2015-06-17T21:06:58Z
Modified the equals method in all the Value type classes
----
---
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: Modified the equals method in all the Value ty...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113303962
I would not expect `equals()` to throw an exception, so this is a good fix in my opinion.
---
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: Modified the equals method in all the Value ty...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/850
---
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: Modified the equals method in all the Value ty...
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113630618
either way, it should be one of either getClass or instanceOf , it is inconsistent right now
---
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: Modified the equals method in all the Value ty...
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113120768
Well yes, my opinion is to use getClass in place of instanceOf , I didnt make the change yet , so in that case I think we need to have a check for null reference.
---
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: Modified the equals method in all the Value ty...
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113121714
instanceOf being an operator will return false if its a null reference , but getClass being a method will give an exception(I think so), also use of getClass makes more sense since its a equals method and we want both the types to be identical(no 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: Modified the equals method in all the Value ty...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-114038819
Merging. Thanks for the PR. Going for getClass.
---
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: Modified the equals method in all the Value ty...
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113119208
So, we have been handling `null` values inconsistently from what I can see. `LongValue` checks for `null` whereas the changed classes in this PR did not. The equals contract is to return `false` for `obj.equals(null)` but I think there are [fair arguments for throwing NPE](http://stackoverflow.com/questions/2887761/is-it-a-bad-idea-if-equalsnull-throws-nullpointerexception-instead) as well.
I think it's good to have it consistent in all `Value` classes. I'm in favor of your change, but it is essentially API breaking. There could be someone out there relying on the NPE. ;-)
Need more opinions on this, @tillrohrmann, @fhueske.
---
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: Modified the equals method in all the Value ty...
Posted by sahitya-pavurala <gi...@git.apache.org>.
Github user sahitya-pavurala commented on the pull request:
https://github.com/apache/flink/pull/850#issuecomment-113235155
If there is someone out there relying on the NPE, i think it should be thrown.(throw)
---
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.
---