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/07/18 15:44:09 UTC
[GitHub] [calcite] hsyuan opened a new pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
hsyuan opened a new pull request #2074:
URL: https://github.com/apache/calcite/pull/2074
Currently the only way to check rel node tree deep equality is transforming
into String by RelOptUtil.toString(rel) with
SqlExplainLevel.EXPPLAN_ATTRIBUTES, which is inefficient. One example is
RexSubQuery. It has to do it this way, because the rel being reference by
RexSubQuery is possibly not yet registered to VolcanoPlanner, and the digest
equals checks the input RelNode by identity (not content). That is OK for
RelSubset and HepRelVertex, if the RelNode is already registered in planner,
but not for plain RelNode that is outside of planner. Due to this, we have to
implement another set of deep equals logic in our system.
With this patch, `digestEquals()` and `digestHash()` are extended to support
deep equality check for RelNode.
CALCITE-4129
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457022817
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Filter.java
##########
@@ -166,13 +166,13 @@ protected boolean digestEquals0(Object obj) {
}
Filter o = (Filter) obj;
return traitSet.equals(o.traitSet)
- && input.equals(o.input)
+ && input.digestEquals(o.input)
&& condition.equals(o.condition)
&& getRowType().equalsSansFieldNames(o.getRowType());
}
@API(since = "1.24", status = API.Status.INTERNAL)
protected int digestHash0() {
- return Objects.hash(traitSet, input, condition);
+ return Objects.hash(traitSet, input.digestHash(), condition);
Review comment:
I don't think we need this.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan closed pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #2074:
URL: https://github.com/apache/calcite/pull/2074
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457007569
##########
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##########
@@ -335,6 +335,26 @@ default String getDigest() {
@API(since = "1.24", status = API.Status.INTERNAL)
void recomputeDigest();
+ /**
+ * Equality check for RelNode digest.
+ *
+ * <p>By default this method collects digest attributes from
+ * explain terms, then compares each attribute pair.</p>
+ *
+ * @return Whether the 2 RelNodes are equivalent or have the same digest.
+ * @see #digestHash()
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ boolean digestEquals(Object obj);
+
+ /**
+ * Compute hash code for RelNode digest.
+ *
+ * @see #digestEquals(Object)
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ int digestHash();
Review comment:
I don't think so. It should be originally part of RelNode, and actually should replace hashCode() (if it isn't final), IMO.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457004889
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -409,17 +407,34 @@ public RelOptTable getTable() {
* @see #digestHash()
*/
@API(since = "1.24", status = API.Status.EXPERIMENTAL)
- protected boolean digestEquals(Object obj) {
+ public boolean digestEquals(Object obj) {
if (this == obj) {
return true;
}
if (this.getClass() != obj.getClass()) {
return false;
}
AbstractRelNode that = (AbstractRelNode) obj;
- return this.getTraitSet().equals(that.getTraitSet())
- && this.getDigestItems().equals(that.getDigestItems())
+ boolean result = this.getTraitSet().equals(that.getTraitSet())
&& this.getRowType().equalsSansFieldNames(that.getRowType());
+ if (!result) {
+ return false;
+ }
+ List<Pair<String, Object>> items1 = this.getDigestItems();
+ List<Pair<String, Object>> items2 = that.getDigestItems();
+ if (items1.size() != items2.size()) {
+ return false;
+ }
+ for (int i = 0; result && i < items1.size(); i++) {
+ Pair<String, Object> attr1 = items1.get(i);
+ Pair<String, Object> attr2 = items2.get(i);
+ if (attr1.right instanceof RelNode) {
+ result = ((RelNode) attr1.right).digestEquals(attr2.right);
+ } else {
+ result = attr1.equals(attr2);
+ }
Review comment:
See L428. The result is checked there.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457001684
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -409,17 +407,34 @@ public RelOptTable getTable() {
* @see #digestHash()
*/
@API(since = "1.24", status = API.Status.EXPERIMENTAL)
- protected boolean digestEquals(Object obj) {
+ public boolean digestEquals(Object obj) {
if (this == obj) {
return true;
}
if (this.getClass() != obj.getClass()) {
return false;
}
AbstractRelNode that = (AbstractRelNode) obj;
- return this.getTraitSet().equals(that.getTraitSet())
- && this.getDigestItems().equals(that.getDigestItems())
+ boolean result = this.getTraitSet().equals(that.getTraitSet())
&& this.getRowType().equalsSansFieldNames(that.getRowType());
+ if (!result) {
+ return false;
+ }
+ List<Pair<String, Object>> items1 = this.getDigestItems();
+ List<Pair<String, Object>> items2 = that.getDigestItems();
+ if (items1.size() != items2.size()) {
+ return false;
+ }
+ for (int i = 0; result && i < items1.size(); i++) {
+ Pair<String, Object> attr1 = items1.get(i);
+ Pair<String, Object> attr2 = items2.get(i);
+ if (attr1.right instanceof RelNode) {
+ result = ((RelNode) attr1.right).digestEquals(attr2.right);
+ } else {
+ result = attr1.equals(attr2);
+ }
Review comment:
we should return here, if the result is false?
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457006511
##########
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##########
@@ -335,6 +335,26 @@ default String getDigest() {
@API(since = "1.24", status = API.Status.INTERNAL)
void recomputeDigest();
+ /**
+ * Equality check for RelNode digest.
+ *
+ * <p>By default this method collects digest attributes from
+ * explain terms, then compares each attribute pair.</p>
+ *
+ * @return Whether the 2 RelNodes are equivalent or have the same digest.
+ * @see #digestHash()
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ boolean digestEquals(Object obj);
+
+ /**
+ * Compute hash code for RelNode digest.
+ *
+ * @see #digestEquals(Object)
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ int digestHash();
Review comment:
Does it make the code clearer by extracting the two methods to a separate interface, as the RelNode interface is already large enough?
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457014805
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Filter.java
##########
@@ -166,13 +166,13 @@ protected boolean digestEquals0(Object obj) {
}
Filter o = (Filter) obj;
return traitSet.equals(o.traitSet)
- && input.equals(o.input)
+ && input.digestEquals(o.input)
&& condition.equals(o.condition)
&& getRowType().equalsSansFieldNames(o.getRowType());
}
@API(since = "1.24", status = API.Status.INTERNAL)
protected int digestHash0() {
- return Objects.hash(traitSet, input, condition);
+ return Objects.hash(traitSet, input.digestHash(), condition);
Review comment:
If we can change RelNode#hashCode(), everything will be normal. I am happy to remove digestHash() if we can remove final from hashCode().
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457800157
##########
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##########
@@ -335,6 +335,26 @@ default String getDigest() {
@API(since = "1.24", status = API.Status.INTERNAL)
void recomputeDigest();
+ /**
+ * Equality check for RelNode digest.
+ *
+ * <p>By default this method collects digest attributes from
+ * explain terms, then compares each attribute pair.</p>
+ *
+ * @return Whether the 2 RelNodes are equivalent or have the same digest.
+ * @see #digestHash()
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ boolean digestEquals(Object obj);
+
+ /**
+ * Compute hash code for RelNode digest.
+ *
+ * @see #digestEquals(Object)
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ int digestHash();
Review comment:
RexNode already supports deep equals.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457022124
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Filter.java
##########
@@ -166,13 +166,13 @@ protected boolean digestEquals0(Object obj) {
}
Filter o = (Filter) obj;
return traitSet.equals(o.traitSet)
- && input.equals(o.input)
+ && input.digestEquals(o.input)
&& condition.equals(o.condition)
&& getRowType().equalsSansFieldNames(o.getRowType());
}
@API(since = "1.24", status = API.Status.INTERNAL)
protected int digestHash0() {
- return Objects.hash(traitSet, input, condition);
+ return Objects.hash(traitSet, input.digestHash(), condition);
Review comment:
I am thinking do we need a utiltiy to combine hash codes and digest hash codes?
So the code can be changed to something like:
`Util.combineHash(traitSet.hashCode(), input.digestHash(), condition.hashCode())`
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457013801
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Filter.java
##########
@@ -166,13 +166,13 @@ protected boolean digestEquals0(Object obj) {
}
Filter o = (Filter) obj;
return traitSet.equals(o.traitSet)
- && input.equals(o.input)
+ && input.digestEquals(o.input)
&& condition.equals(o.condition)
&& getRowType().equalsSansFieldNames(o.getRowType());
}
@API(since = "1.24", status = API.Status.INTERNAL)
protected int digestHash0() {
- return Objects.hash(traitSet, input, condition);
+ return Objects.hash(traitSet, input.digestHash(), condition);
Review comment:
It is weird we have to combine hash code with objects.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2074: [CALCITE-4129] Support deep equality check for RelNode
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2074:
URL: https://github.com/apache/calcite/pull/2074#discussion_r457021057
##########
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##########
@@ -335,6 +335,26 @@ default String getDigest() {
@API(since = "1.24", status = API.Status.INTERNAL)
void recomputeDigest();
+ /**
+ * Equality check for RelNode digest.
+ *
+ * <p>By default this method collects digest attributes from
+ * explain terms, then compares each attribute pair.</p>
+ *
+ * @return Whether the 2 RelNodes are equivalent or have the same digest.
+ * @see #digestHash()
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ boolean digestEquals(Object obj);
+
+ /**
+ * Compute hash code for RelNode digest.
+ *
+ * @see #digestEquals(Object)
+ */
+ @API(since = "1.25", status = API.Status.EXPERIMENTAL)
+ int digestHash();
Review comment:
I see. Thanks for your clarification.
Do we intend to support this for other tree-like data structures (e.g. RexNode) ?
----------------------------------------------------------------
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