You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by hy...@apache.org on 2020/06/23 14:22:37 UTC
[calcite] branch master updated: [CALCITE-4083] RelTraitSet failed
to canonize traits
This is an automated email from the ASF dual-hosted git repository.
hyuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new 0769a8b [CALCITE-4083] RelTraitSet failed to canonize traits
0769a8b is described below
commit 0769a8b31cbbeb5bca66ade30cf3710523da4aaa
Author: Haisheng Yuan <h....@alibaba-inc.com>
AuthorDate: Mon Jun 22 17:14:34 2020 -0500
[CALCITE-4083] RelTraitSet failed to canonize traits
RelTraitSet#plus uses FlatLists and ImmutableList. They have different hash
algorithms, and they are all different classes from RelTraitSet. The
RelTraitSet equality requires the other object must be RelTraitSet too, and the
HashMap#get(key) uses key.equals() to check equality, instead of the other way.
In case we pass RelTraitSet as the search key to cache, but the cached entry
has key with FlatLists or ImmutableList, we may fail to find the cached
RelTraitSet. Due to this, using == to check traitSet equality may fail, even
they have same traits.
Also modified hashCode() of RelDataTypeFieldImpl, the original implementation
generates hash collision easily.
Close #2041
---
.../java/org/apache/calcite/plan/RelTraitSet.java | 52 ++++++----------------
.../org/apache/calcite/plan/volcano/RelSet.java | 2 +-
.../calcite/rel/type/RelDataTypeFieldImpl.java | 5 +--
3 files changed, 16 insertions(+), 43 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/plan/RelTraitSet.java b/core/src/main/java/org/apache/calcite/plan/RelTraitSet.java
index c521968..22c59b1 100644
--- a/core/src/main/java/org/apache/calcite/plan/RelTraitSet.java
+++ b/core/src/main/java/org/apache/calcite/plan/RelTraitSet.java
@@ -20,7 +20,6 @@ import org.apache.calcite.rel.RelCollation;
import org.apache.calcite.rel.RelCollationTraitDef;
import org.apache.calcite.rel.RelDistribution;
import org.apache.calcite.rel.RelDistributionTraitDef;
-import org.apache.calcite.runtime.FlatLists;
import org.apache.calcite.util.mapping.Mappings;
import com.google.common.collect.ImmutableList;
@@ -42,9 +41,9 @@ public final class RelTraitSet extends AbstractList<RelTrait> {
private final Cache cache;
private final RelTrait[] traits;
- private final String string;
+ private String string;
/** Cache the hash code for the traits */
- private int hash = 0;
+ private int hash; // Default to 0
//~ Constructors -----------------------------------------------------------
@@ -61,7 +60,6 @@ public final class RelTraitSet extends AbstractList<RelTrait> {
// the caller has made a copy.
this.cache = cache;
this.traits = traits;
- this.string = computeString();
}
//~ Methods ----------------------------------------------------------------
@@ -561,6 +559,9 @@ public final class RelTraitSet extends AbstractList<RelTrait> {
}
@Override public String toString() {
+ if (string == null) {
+ string = computeString();
+ }
return string;
}
@@ -620,29 +621,12 @@ public final class RelTraitSet extends AbstractList<RelTrait> {
if (i >= 0) {
return replace(i, trait);
}
- // Optimize time & space to represent a trait set key.
- //
- // Don't build a trait set until we're sure there isn't an equivalent one.
- // Then we can justify the cost of computing RelTraitSet.string in the
- // constructor.
final RelTrait canonizedTrait = canonize(trait);
assert canonizedTrait != null;
- List<RelTrait> newTraits;
- switch (traits.length) {
- case 0:
- newTraits = ImmutableList.of(canonizedTrait);
- break;
- case 1:
- newTraits = FlatLists.of(traits[0], canonizedTrait);
- break;
- case 2:
- newTraits = FlatLists.of(traits[0], traits[1], canonizedTrait);
- break;
- default:
- newTraits = ImmutableList.<RelTrait>builder().add(traits)
- .add(canonizedTrait).build();
- }
- return cache.getOrAdd(newTraits);
+ RelTrait[] newTraits = new RelTrait[traits.length + 1];
+ System.arraycopy(traits, 0, newTraits, 0, traits.length);
+ newTraits[traits.length] = canonizedTrait;
+ return cache.getOrAdd(new RelTraitSet(cache, newTraits));
}
public RelTraitSet plusAll(RelTrait[] traits) {
@@ -704,24 +688,14 @@ public final class RelTraitSet extends AbstractList<RelTrait> {
/** Cache of trait sets. */
private static class Cache {
- final Map<List<RelTrait>, RelTraitSet> map = new HashMap<>();
+ final Map<RelTraitSet, RelTraitSet> map = new HashMap<>();
Cache() {
}
- RelTraitSet getOrAdd(List<RelTrait> traits) {
- RelTraitSet traitSet1 = map.get(traits);
- if (traitSet1 != null) {
- return traitSet1;
- }
- final RelTraitSet traitSet;
- if (traits instanceof RelTraitSet) {
- traitSet = (RelTraitSet) traits;
- } else {
- traitSet = new RelTraitSet(this, traits.toArray(new RelTrait[0]));
- }
- map.put(traits, traitSet);
- return traitSet;
+ RelTraitSet getOrAdd(RelTraitSet t) {
+ RelTraitSet exist = map.putIfAbsent(t, t);
+ return exist == null ? t : exist;
}
}
}
diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
index 09e6f37..833aeba 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
@@ -159,7 +159,7 @@ class RelSet {
public RelSubset getSubset(RelTraitSet traits) {
for (RelSubset subset : subsets) {
- if (subset.getTraitSet() == traits) {
+ if (subset.getTraitSet().equals(traits)) {
return subset;
}
}
diff --git a/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFieldImpl.java b/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFieldImpl.java
index 8af3422..4d1ac98 100644
--- a/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFieldImpl.java
+++ b/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFieldImpl.java
@@ -19,6 +19,7 @@ package org.apache.calcite.rel.type;
import org.apache.calcite.sql.type.SqlTypeName;
import java.io.Serializable;
+import java.util.Objects;
/**
* Default implementation of {@link RelDataTypeField}.
@@ -49,9 +50,7 @@ public class RelDataTypeFieldImpl implements RelDataTypeField, Serializable {
//~ Methods ----------------------------------------------------------------
@Override public int hashCode() {
- return index
- ^ name.hashCode()
- ^ type.hashCode();
+ return Objects.hash(index, name, type);
}
@Override public boolean equals(Object obj) {