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/06/21 00:57:21 UTC

[GitHub] [calcite] hsyuan opened a new pull request #2039: Digest on RelNode and RexCall

hsyuan opened a new pull request #2039:
URL: https://github.com/apache/calcite/pull/2039


   See discussion in CALCITE-3786.


----------------------------------------------------------------
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] danny0405 commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647564909


   > @danny0405
   > What you mean by "get involve in the discussion in the [CALCITE-3786](https://issues.apache.org/jira/browse/CALCITE-3786) first";
   > I checked this PR and give my approve. From my point it's indeed much more concise than `Digest` in the current master. And it also gives enough consideration on backward compatibility.
   
   I have replied in the issue, please also move 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] hsyuan commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647540418


   > Just one question, if the RelDigest is regarded as internal, why not it's not an inner class in AbstractRelNode ?
   
   Planner also use it, they are not in the same package. So I think we don't have choice.


----------------------------------------------------------------
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] jinxing64 commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
jinxing64 commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647526751


   @danny0405 
   What you mean by "get involve in the discussion in the CALCITE-3786 first";
   I checked this PR and give my approve. From my point it's indeed much more concise than `Digest` in the current master. And it also gives enough consideration on backward compatibility.


----------------------------------------------------------------
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 pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647238259


   Comparing with version 1.23.0, it is.
   Comparing with current master, it is a less broken change.


----------------------------------------------------------------
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] jinxing64 commented on a change in pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#discussion_r443469463



##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptNode.java
##########
@@ -45,9 +47,20 @@
    * <p>If you want a descriptive string which contains the identity, call
    * {@link Object#toString()}, which always returns "rel#{id}:{digest}".
    *
+   * @return Digest string of this {@code RelNode}
+   */

Review comment:
       Thanks a lot to keep this `String getDigest`, which is heavily relied on in our internal code base.




----------------------------------------------------------------
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 merged pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
hsyuan merged pull request #2039:
URL: https://github.com/apache/calcite/pull/2039


   


----------------------------------------------------------------
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] XuQianJin-Stars commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
XuQianJin-Stars commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647237652


   Features +1，It seems a bit broken here(RelDigest)?


----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#discussion_r443597562



##########
File path: core/src/main/java/org/apache/calcite/rex/RexSubQuery.java
##########
@@ -139,11 +138,14 @@ public RexSubQuery clone(RelNode rel) {
 
   @Override public boolean equals(Object obj) {
     return obj == this
-        || obj instanceof RexCall
+        || obj instanceof RexSubQuery
         && toString().equals(obj.toString());

Review comment:
       Nice catch




----------------------------------------------------------------
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] yanlin-Lynn commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647536605


   Actually, I think the implementation here do not have much difference with the master, just with moving the Digest related logic to InnerRelDigest.
   +1 for the concise and backward compatibility. 


----------------------------------------------------------------
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] danny0405 commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647505206


   > +1 on this and thanks a lot @hsyuan, RelDigest is much concise.
   > Just one question, if the `RelDigest` is regarded as internal, why not it's not an inner class in AbstractRelNode ?
   
   If possible, please get involve in the discussion in the CALCITE-3786 first.


----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#discussion_r443589712



##########
File path: core/src/main/java/org/apache/calcite/plan/RelDigest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.plan;
+
+import org.apache.calcite.rel.RelNode;
+
+import org.apiguardian.api.API;
+
+/**
+ * The digest is the exact representation of the corresponding {@code RelNode},
+ * at anytime, anywhere. The only difference is that digest is compared using
+ * {@code #equals} and {@code #hashCode}, which are prohibited to override
+ * for RelNode, for legacy reasons.
+ *
+ * <p>INTERNAL USE ONLY.</p>
+ */
+@API(since = "1.24", status = API.Status.INTERNAL)
+public interface RelDigest {
+  /**
+   * Reset state, possibly cache of hash code.
+   */
+  void clear();
+
+  /**
+   * Returns the relnode that this digest is associated with.
+   */

Review comment:
       Any benchmark for the improvement?




----------------------------------------------------------------
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] jinxing64 commented on pull request #2039: Digest on RelNode and RexCall

Posted by GitBox <gi...@apache.org>.
jinxing64 commented on pull request #2039:
URL: https://github.com/apache/calcite/pull/2039#issuecomment-647428693


   +1 on this and thanks a lot @hsyuan, RelDigest is much concise.
   Just one question, if the `RelDigest` is regarded as internal, why not it's not an inner class in AbstractRelNode ? 


----------------------------------------------------------------
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