[CALCITE-3786] Add Digest interface to enable efficient hashCode(equa… by danny0405 · Pull Request #2016 · apache/calcite

danny0405 · 2020-06-09T13:21:55Z

…ls) for RexNode and RelNode

Add class Digest used to identify the node;
There is a pre-computed hashcode to speedup #hashCode and #equals;
We still use the string digest comparison for RexNode, which can be
promoted as reused objects later

vlsi · 2020-06-09T13:34:46Z

+    return Objects.hash(collect(rel, contents));
+  }
+
+  private static Object[] collect(RelNode rel, List<Pair<String, Object>> contents) {


This seems to be resource-consuming. Should the result is not cached in the constructor?

This is used to resolve the hash code conflict, which expects to be rare case, the cache would not bring much.

equals is always executed (== in case hashCode matches), and equals would be called multiple times in case of a hash collision, so it is worth making both equals and hashCode fast.

I run all the cases and there are only 8 tests that have hash conflict, i would try to promote.

Finally i decide to keep it as is, because the conflict is very few, i also remove the row type when computing hashcode, because row type diff for equiv node is also rare case(usually because of nullability changes).

laurentgo

I have to say that I don't see how this pull request makes hash code more efficient (precomputation of hashcode?). For sure it looks to increase memory usage and also remove lots of flexibility for Calcite users extending on RelNode and RexnNode...

laurentgo · 2020-06-12T15:57:40Z

+  }
+
+  @Override public int compareTo(Digest other) {
+    return this.rel.getId() - other.rel.getId();


This breaks the contract that if 2 objects are equal, then the method should return 0.

This is only for debugging, strictly to say, i have no idea how to compare 2 nodes that are semantically equivalent (#equals) while have different members.

why is Comparable needed for debugging? Right now, I don't see how debuggability can justify breaking the contract

Well, i have fixed the consistency of #equals.

Maybe it would have been simpler not to implement Comparable? You didn't clarify why this is needed for debugging purposes...

That was proposed initially by Julian in the issue, i think it is needed when we want to sort on that.

laurentgo · 2020-06-12T16:01:17Z

+  private boolean deepEquals(Digest other) {
+    Object[] thisItems = collect(this.rel, this.items, true);
+    Object[] thatItems = collect(other.rel, other.items, true);
+    if (thisItems.length != thatItems.length) {


The whole check + loop could be replaced with Arrays.equals(Object[], Object[])

Sorry, i would choose current code because it's simpler.

are you telling that 9 lines are simpler that one line of code which is return Arrays.equals(thisItems, thatItems)?

Did you see the code in Arrays.equals(thisItems, thatItems) ? Here the thisItems and thatItems are both never null.

Considering null checks are often optimized out by the JVM, I'm not sure that the possible extra optimization is worth the extra code you wrote...

It is often but not always, if you are not sure also, let's keep it as it is, which is also straight-forward code.

Sorry, I am at a loss for arguments. The original reply was that the code was simpler than Arrays.equals(Object[], Object[]), a standard JDK method, whereas in fact, it is basically the same code, except for the check for null, which is often suppressed by the JVM compiler (cf https://shipilev.net/jvm/anatomy-quarks/25-implicit-null-checks/) and even so has minimal impact compared to the two calls to the #collect method where a list is created and converted later into an array...

The test here #2034 shows that there is negligible performance affect with ArrayList.toArray, so still, i would not follow you.

laurentgo · 2020-06-12T16:04:32Z

+      return false;
+    }
+    for (int i = 0; i < thisItems.length; i++) {
+      if (!Objects.equals(thisItems[i], thatItems[i])) {


should Arrays.deepEquals be used if thisItems[i] is an array?

I checked all the rels, they all have no array attributes in the digest, even it has, the #equals would output false which does not affect the semantics. So i choose current impl.

for now? what about external rels?

I said outputs false does not affect the semantics, then what's the problem there, i do not want to make the logic a mess just for a non-sure assumption.

If you have strong opinion, please give specific cases, only assumption makes no sense to me.

Yes, there has some "ifs" there. There are 2 assumptions here as well:

there's no rel node using an array as a digest field, which is true for Calcite code, and probably true for code using the library although it's harder to verify

even so, return false when 2 array contents are identical has no impact on the semantic.

For the first point, maybe a more opinionated choice could be to disallow arrays as digest item?
For the second point, I'm not sure that the assumption holds: if planner cannot identify that 2 rel nodes are identical (and so collapse them), it might cause some kind of infinite loop/plan explosion where the same node is created over and over.

I don't think we should have any limitation on what should be a digest item just because of the digest code implementation

Did we encounter any case that has infinite loop just because of an array item there ? I need to see the case to make decision, not an assumption. I prefer to fix that when we really encounter that, i think such case would be very rare.

laurentgo · 2020-06-12T16:09:54Z

+  private static String simpleRelDigest(RelNode rel) {
+    StringBuilder digest = new StringBuilder(rel.getRelTypeName());
+    digest.append('#');
+    if (rel instanceof RelSubset) {


Why not let the rel node itself handle how to create the digest vs having a non-extendable/overridable method using different behaviors based on the type of rel to do it? This looks like an anti design pattern.

For current impl, the digest is handled by each rel node. Split the code everywhere is hard to maintain, let the Digest take care how the digest should be generated but the invoker is still each node.

Considering that method, the digest takes care how the digest should be generated for nodes it already knows about. Based on the current approach, if I create a new RelNode subclass in my project which has a very natural/efficient way of creating a digest, I would have no choice than to fork the Calcite project to modify that method.
I would favor an approach which would let HepRelVertex itself decides what to use for its digest instead of hardcoding in this method that the current rel should be chosen.

RelNode subclass in my project which has a very natural/efficient way of creating a digest, I would have no choice than to fork the Calcite project to modify that method

Did you notice that the #computeDigest and #explainTerms are both API for overriding the digest ?

HepRelVertex is not a public API, i don't think there is necessity to customize its digest, if you have a sub-class, override the #computeDigest is till feasible.

why not using the #computeDigest with the vertex id then?

Also it is not possible to "substract" to the default digest. One can only add. The patch you proposed has some behavior which is not possible to change

The current patch also supports pure string digest, which is same as before, if you think it is impossible, please give an example.

The digest of HepRelVertex and RelSubSet are the same as before, what do you mean by using the #computeDigest with the vertex id then ?

I guess I missed some update to your change (or missed the second constructor), but it looks like the choice is either:

provide rel + a list of extra items, and the logic to create the hashcode + digest is totally controlled by the digest class

provide rel + digest string to skip the whole hash computation logic

I guess nothing in-between...

The digest of HepRelVertex and RelSubSet are the same as before, what do you mean by using the #computeDigest with the vertex id then ?
It's back to my comment about having the logic hardcoded into a single private static method which goes against OOP practices whereas instructing HelpRexVertex and RelSubSet instructing digest that the only field to consider for digest should be id

Use the digest string to compute hashcode works well then the string is short, you can use the benchmark #2034 to see the diff. The performance diff is negligible.

danny0405 · 2020-06-13T00:52:56Z

I have to say that I don't see how this pull request makes hash code more efficient (precomputation of hashcode?). For sure it looks to increase memory usage and also remove lots of flexibility for Calcite users extending on RelNode and RexnNode...

Compare to the old code, this patch reduce the memory usage, especially for the RexNode. What do you mean by remove lots of flexibility for Calcite users ? Which flexibility do i remove ?

…ls) for RexNode and RelNode * Add class Digest used to identify the node; * There is a pre-computed hashcode to speedup #hashCode and #equals; * Change RexCall to use object#equals instead of pure string digest comparison; * We only support RexInputRef normalization which is the most common case; * Remove RexNode#toStringRaw because it makes the thing complicated, RexNode can always be normalized(default true).

laurentgo · 2020-06-16T04:13:42Z

Regarding my concern about memory usage, I was talking about Digest and the extra list used to store items which is kind of redundant with the RelNode itself. An alternative could have to replace the list of extra fields with an Iterable and call it each time items need to be collected? It could also possibly eliminate the creation of an array to do a comparison between objects...

danny0405 · 2020-06-16T08:37:22Z

Regarding my concern about memory usage, I was talking about Digest and the extra list used to store items which is kind of redundant with the RelNode itself. An alternative could have to replace the list of extra fields with an Iterable and call it each time items need to be collected? It could also possibly eliminate the creation of an array to do a comparison between objects...

I have gave the explanation in the JIRA issue about why i keep redundant references of the RelNode items, that still makes sense to me.

An alternative could have to replace the list of extra fields with an Iterable

It is possible, if you think there is also some thing can do that can promote the Digest code, go for it and very appreciate for that.

danny0405 · 2020-06-18T10:14:18Z

Regarding my concern about memory usage, I was talking about Digest and the extra list used to store items which is kind of redundant with the RelNode itself. An alternative could have to replace the list of extra fields with an Iterable and call it each time items need to be collected? It could also possibly eliminate the creation of an array to do a comparison between objects...

What do you mean by extra fields ? If possible, can you give a POC code there, the discussion with all kinds of assumptions make me burden out.

And if possible, use the code here #2034 to illustrate that you are right.

chunweilei · 2020-06-19T02:29:02Z

+  @Override protected Digest computeDigest() {
+    return Digest.create(this, getRelTypeName() + '#' + getCurrentRel().getId());
  }



Why do you use getRelTypeName() rather than "HepRelVertex"?

To unify the logic, we can switch to "HepRelVertex", that's a preference.

chunweilei · 2020-06-19T02:41:43Z

+            + "PLAN=EnumerableCalc(expr#0..3=[{inputs}], empid=[$t1], name=[$t3])\n"
            + "  EnumerableCorrelate(correlation=[$cor1], joinType=[inner], requiredColumns=[{0}])\n"
            + "    EnumerableAggregate(group=[{0}])\n"
            + "      EnumerableTableScan(table=[[s, depts]])\n"


It's weird to add PALN= just for the two cases. It's better to have the same format for all the cases.

I think we could, thanks ~

chunweilei · 2020-06-19T03:01:24Z

+  public static Digest initial(RelNode rel) {
+    return new Digest(rel, simpleRelDigest(rel));
+  }
+}


What's the difference between Digest#create and Digest#initial ? Their results seem the same.

To distinguish that the digest is just as a initial which should only be used once in the constructor.

It's really hard to distinguish these two methods. Is it possible to remove Digest#create which seems useless?

I can remove the #initial and we use the #create uniformly.

chunweilei · 2020-06-19T03:09:59Z

+      sb.append('.');
+      sb.append(trait.toString());
+    }
+


As you said "List<Pair<String, Object>> items are the properties, e.g. the inputs, the type, the traits and so on", why should we still have to append trait?

The properties do not include traits, the Digest does.

Please correct the comment.

chunweilei · 2020-06-19T03:20:17Z

    @Override public int hashCode() {
-      return Objects.hash(digest, ordinal, distinct);
+      return Objects.hash(super.hashCode(), ordinal, distinct, ignoreNulls);
    }


Nice catch.

vlsi reviewed Jun 9, 2020

View reviewed changes

danny0405 force-pushed the CALCITE-3786 branch 2 times, most recently from 8284356 to a51dc07 Compare June 12, 2020 05:39

danny0405 added the slow-tests-needed label Jun 12, 2020

danny0405 force-pushed the CALCITE-3786 branch 2 times, most recently from 02eebf9 to 545743d Compare June 12, 2020 08:24

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jun 12, 2020

laurentgo requested changes Jun 12, 2020

View reviewed changes

danny0405 force-pushed the CALCITE-3786 branch 4 times, most recently from 09bd694 to ba5d947 Compare June 16, 2020 02:29

danny0405 force-pushed the CALCITE-3786 branch from ba5d947 to 380cc44 Compare June 16, 2020 02:34

danny0405 merged commit 69f2586 into apache:master Jun 16, 2020

chunweilei reviewed Jun 19, 2020

View reviewed changes

Uh oh!

Conversation

danny0405 commented Jun 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurentgo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurentgo Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 commented Jun 13, 2020

Uh oh!

laurentgo commented Jun 16, 2020

danny0405 Jun 15, 2020 •

edited

Loading

laurentgo Jun 16, 2020 •

edited

Loading

danny0405 commented Jun 18, 2020 •

edited

Loading

chunweilei Jun 19, 2020 •

edited

Loading