impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tianyi Wang (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5976: Remove equivalence class computation in FE
Date Wed, 15 Nov 2017 00:31:45 GMT
Tianyi Wang has posted comments on this change. ( )

Change subject: IMPALA-5976: Remove equivalence class computation in FE

Patch Set 6:

File fe/src/main/java/org/apache/impala/analysis/
PS3, Line 91: /**
> Fair point. I was thinking of a definition like this:
"containing both slots" only covers join.
File fe/src/main/java/org/apache/impala/analysis/
PS4, Line 102:  * Slot value transfers:
> Slot value transfers:
PS4, Line 105:  * Each slot is contained in exactly one equivalence class. A slot equivalence
class is a
> What does "Its" refer to here?
Done. The first 'its' is the value transfer relation. The second 'its' is the symmetric closure.
PS4, Line 1527:       // select * from (select A.a, B.b from A left join B on A.a=B.b) v where
v.b is null
> to drive it home even further use "B.col is null" as the predicate to show 
Do you mean "v.b is null"?
PS4, Line 1648:     // A map from equivalence class IDs to equivalence classes. The equivalence
> typo: classes
PS4, Line 1813:    * Returns a map of slot equivalence classes on the set of slots in the
given tuples.
> the given tuples
PS4, Line 1838:    * propagation. Each mapping assigns every slot in srcSids to a slot in
destTid which
> Each mapping assigns every slot in srcSids to a slot in destTid which has a
PS4, Line 1989:             "Condensed Graph:\n" + condensedTc);
> move the first "\n" to previous line
PS4, Line 1998:     for (ExprId id : globalState_.conjuncts.keySet()) {
> remove (pretty clear from function comment)
File fe/src/main/java/org/apache/impala/analysis/
PS4, Line 691:    * Returns true if this expr matches 'that'. Two exprs match if:
> Move this into SlotRef since it's SlotRef specific?
PS4, Line 696:    */
> Returns true if this expr matches 'that'
PS4, Line 699:     if (this instanceof CastExpr && ((CastExpr)this).isImplicit())
> For every pair of corresponding SlotRefs, slotRefCmp.matches() returns true
PS4, Line 700:       return children_.get(0).matches(that, slotRefCmp);
> localEquals()
PS4, Line 719:   protected boolean localEquals(Expr that) {
> I think we should separate matches() and localEquals() more cleanly. I thin
PS4, Line 727:    */
> Not checking fn_ seems a little strange. The function semantics would be cl
PS4, Line 962:         if (newExpr.matches(expr, cmp)) {
> according to matches() using 'cmp'
File fe/src/main/java/org/apache/impala/analysis/
PS4, Line 198: 
> A wrapper around localEquals().
File fe/src/main/java/org/apache/impala/planner/
PS3, Line 399:     DataPartition outputPartition;
> Makes sense, please add a comment to explain.
Done. I just realized that full outer joins should have random output partition. I added a
test case in the planner test. That test case breaks both ps4 and the base.
File fe/src/main/java/org/apache/impala/util/
PS3, Line 52:     private final IntArrayList[] adjList_;
> This addresses the performance issue, but it does not address the testing/r
File fe/src/main/java/org/apache/impala/util/
PS4, Line 91: 
> @Override
PS4, Line 106:     /**
> Don't understand the second sentence
That sentence is removed.
PS4, Line 134:               visited.set(dstIt.peek());
> A graph condensed by its strongly-connected components (SCC). Vertices are 
PS4, Line 152:    * their SCCs and an inner graph on the SCCs is stored.
> @Override
PS4, Line 199:         public int peek() {
> Very clear now! Thanks.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4cb1d8be46efa8fd61a97048cc79dabe2ffa51a
Gerrit-Change-Number: 8317
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Comment-Date: Wed, 15 Nov 2017 00:31:45 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message