You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/28 07:21:18 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #1839: [CALCITE-3836]
The hash codes of RelNodes are unreliable
liyafan82 opened a new pull request #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839
For all sub-classes of AbstractRelNode, the hashCode methods depend on AbstractRelNode#hashCode, because it is declared as final.
AbstractRelNode#hashCode depends on Object#hashCode, which is called identify hash code. The details of identity hash code depends on the specific JVM implementation. For many JVMs, the implementation is based on the object address in the memory. The problem is that, the address of an object may change in a JVM, due to GC, memory contraction, etc. So the hash code of an object may change, even if the content of the object is not changed (This can be confirmed from the JavaDoc of Object#hashCode).
This problem may cause severe issues that are hard to diagnose and debug, like an object is in the hash table, but cannot be retrieved; duplicate objects in the hash map, etc.
To solve the problem, we compute a hash code solely from the node id. This is consistent with the previous semantics, and solves the above problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi edited a comment on issue #1839: [CALCITE-3836] The
hash codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-595117753
> liyafan82: So the hash code of an object may change
> Vladimir: This is false
> liyafan82: Would you please give some docs/link/code proving this?
@liyafan82 , would you please provide docs/link/code instead?
Here's the javadoc for [Object#hashCode](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#hashCode--)
>Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method **_must consistently return the same integer_**
It is sad your statement contradicts the very basic javadoc for `Object#hashCode`, and you ask others to prove something.
It is you who should provide enough evidence to justify the bug exists.
---
> liyafan82: So the hash code of an object may change
Can you please provide a justification or a reproducible case instead?
> liyafan82: Sorry. I do not understand why it violates the contract. Would you please elaborate more on this?
Please check the javadoc for `Object#hashCode` and `#equals`. Please study https://jqno.nl/equalsverifier/ as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 edited a comment on issue #1839: [CALCITE-3836]
The hash codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
liyafan82 edited a comment on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-593186192
> TL;DR: it looks like currently there's no justification for the change :-/
>
> I'm sorry to disappoint you, but
>
> > For many JVMs, the implementation is based on the object address in the memory
>
> This is false.
Would you please give some docs/link/code proving this?
>
> > So the hash code of an object may change
>
> This is false.
Would you please give some docs/link/code proving this?
>
> > This is consistent with the previous semantics, and solves the above problem.
>
> The new implementation violates `equal` and `hashCode` contract :-/
Sorry. I do not understand why it violates the contract. Would you please elaborate more on this?
@vlsi Thanks a lot for your feedback. Looking forward to more feedback from you :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1839: [CALCITE-3836] The hash
codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-593175950
If the rel node has the same id, that means it is the same object, since each rel has distinct id. So the default hashcode() serves its purpose. But I don't remember there is any place using RelNode as the hash map keys.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1839: [CALCITE-3836] The
hash codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-593186192
> TL;DR: it looks like currently there's no justification for the change :-/
>
> I'm sorry to disappoint you, but
>
> > For many JVMs, the implementation is based on the object address in the memory
>
> This is false.
Would you please give some docs/link/code proving this?
>
> > So the hash code of an object may change
>
> This is false.
Would you please give some docs/link/code proving this?
>
> > This is consistent with the previous semantics, and solves the above problem.
>
> The new implementation violates `equal` and `hashCode` contract :-/
Sorry. I do not understand why it violates the contract. Would you please elaborate more on this?
@vlsi Thanks a lot for your feedback. Looking forward for more feedback from you :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1839: [CALCITE-3836] The hash
codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-595117753
> liyafan82: So the hash code of an object may change
> Vladimir: This is false
> liyafan82: Would you please give some docs/link/code proving this?
@liyafan82 , would you please provide docs/link/code instead?
Here's the javadoc for [Object#hashCode](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#hashCode--)
>Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method **_must consistently return the same integer_**
> liyafan82: So the hash code of an object may change
Can you please provide a justification or a reproducible case instead?
> liyafan82: Sorry. I do not understand why it violates the contract. Would you please elaborate more on this?
Please check the javadoc for `Object#hashCode` and `#equals`. Please study https://jqno.nl/equalsverifier/ as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1839: [CALCITE-3836] The hash
codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-592712216
TL;DR: it looks like currently there's no justification for the change :-/
I'm sorry to disappoint you, but
> For many JVMs, the implementation is based on the object address in the memory
This is false.
> So the hash code of an object may change
This is false.
> This is consistent with the previous semantics, and solves the above problem.
The new implementation violates `equal` and `hashCode` contract :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1839: [CALCITE-3836] The
hash codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-593187160
> If the rel node has the same id, that means it is the same object, since each rel has distinct id. So the default hashcode() serves its purpose. But I don't remember there is any place using RelNode as the hash map keys.
@hsyuan Thanks a lot for your feedback. One example that comes to my mind is the HepPlanner. It uses a DefaultDirectedGraph internally, and in the DefaultDirectedGraph, the HepVertex is used as key of the hash map.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1839: [CALCITE-3836] The
hash codes of RelNodes are unreliable
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1839: [CALCITE-3836] The hash codes of RelNodes are unreliable
URL: https://github.com/apache/calcite/pull/1839#issuecomment-595131405
> > liyafan82: So the hash code of an object may change
> > Vladimir: This is false
> > liyafan82: Would you please give some docs/link/code proving this?
>
> @liyafan82 , would you please provide docs/link/code instead?
>
> Here's the javadoc for [Object#hashCode](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#hashCode--)
>
> > Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method **_must consistently return the same integer_**
>
> > liyafan82: So the hash code of an object may change
>
> Can you please provide a justification or a reproducible case instead?
>
> > liyafan82: Sorry. I do not understand why it violates the contract. Would you please elaborate more on this?
>
> Please check the javadoc for `Object#hashCode` and `#equals`. Please study https://jqno.nl/equalsverifier/ as well.
@vlsi Thanks a lot for your feedback. I am aware of the JavaDocs. I am just a little concerned about the implementation details. According to the discussio in the JIRA, I think I am convinced that the hashCode should not change.
The tool you recommended is cool. Thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services