Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistent equals and hashCode #8381

Merged
merged 4 commits into from Sep 4, 2019

Conversation

@asdf2014
Copy link
Member

commented Aug 23, 2019

Description

Fix inconsistent equals and hashCode, more details: lgtm.com


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • added integration tests.
  • been tested in a test Druid cluster.

@Override
public int hashCode()
{
return Objects.hash(getBucket(), getPath());

This comment has been minimized.

Copy link
@gianm

gianm Aug 23, 2019

Contributor

It doesn't matter too much, but might as well do bucket and path here to match equals.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Aug 23, 2019

Author Member

Done 👍

@Override
public int hashCode()
{
return Objects.hash(getName(), getFieldName(), getEstimator(), isVariancePop);

This comment has been minimized.

Copy link
@gianm

gianm Aug 23, 2019

Contributor

Might as well use direct field accesses, to match what equals does.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Aug 23, 2019

Author Member

Done 👍

@Override
public int hashCode()
{
return super.hashCode();

This comment has been minimized.

Copy link
@gianm

gianm Aug 23, 2019

Contributor

This isn't correct; it defers to the default implementation of Object::hashCode, which will be different for different instances, but the contract of hashCode requires that two objects' hash codes be the same if equals is true.

It would be better to return a constant.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Aug 23, 2019

Author Member

Oops.. Fixed.

@Override
public int hashCode()
{
return super.hashCode();

This comment has been minimized.

Copy link
@gianm

gianm Aug 23, 2019

Contributor

I think this has the same problem as IdentityExtractionFn. It should return a constant rather than super.hashCode().

Please check the other usages of super.hashCode() as well.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Aug 23, 2019

Author Member

Oops.. Fixed.

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 23, 2019

This pull request fixes 12 alerts when merging 5d90021 into 661976f - view on LGTM.com

fixed alerts:

  • 12 for Inconsistent equals and hashCode
@lgtm-com

This comment has been minimized.

Copy link

commented Aug 23, 2019

This pull request fixes 12 alerts when merging 0708ac7 into d117bfb - view on LGTM.com

fixed alerts:

  • 12 for Inconsistent equals and hashCode

@asdf2014 asdf2014 force-pushed the asdf2014:fix_hashcode branch from 81c071e to 369b36e Aug 28, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 28, 2019

This pull request fixes 12 alerts when merging 369b36e into 7afe473 - view on LGTM.com

fixed alerts:

  • 12 for Inconsistent equals and hashCode
@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@gianm PTAL.

@Override
public int hashCode()
{
return super.hashCode();

This comment has been minimized.

Copy link
@jihoonson

jihoonson Sep 4, 2019

Contributor

Hmm, is it better to remove equals() from InsensitiveContainsSearchQuerySpec since it's just a syntax sugar for ContainsSearchQuerySpec with the false caseSensitive? Like, given a ContainsSearchQuerySpec with the false caseSensitive and an InsensitiveContainsSearchQuerySpec, should they equal to each other? I think they should be same.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Sep 4, 2019

Author Member

I agree with you, removed. 👍

@jihoonson
Copy link
Contributor

left a comment

Thanks for the quick fix! +1 after CI

@lgtm-com

This comment has been minimized.

Copy link

commented Sep 4, 2019

This pull request fixes 12 alerts when merging 3234cb3 into ee4ebb4 - view on LGTM.com

fixed alerts:

  • 12 for Inconsistent equals and hashCode

@asdf2014 asdf2014 merged commit de18840 into apache:master Sep 4, 2019

3 of 5 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Inspections: pull requests (Druid) TeamCity build finished
Details
LGTM analysis: Java 12 fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:fix_hashcode branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.