Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8317 )
Change subject: IMPALA-5976: Remove equivalence class computation in FE ...................................................................... Patch Set 6: (24 comments) http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@91 PS3, Line 91: /** > Fair point. I was thinking of a definition like this: "containing both slots" only covers join. http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@102 PS4, Line 102: * Slot value transfers: > Slot value transfers: Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@105 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. http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1527 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"? http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1648 PS4, Line 1648: // A map from equivalence class IDs to equivalence classes. The equivalence classes > typo: classes Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1813 PS4, Line 1813: * Returns a map of slot equivalence classes on the set of slots in the given tuples. > the given tuples Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1838 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 Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1989 PS4, Line 1989: "Condensed Graph:\n" + condensedTc); > move the first "\n" to previous line Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1998 PS4, Line 1998: for (ExprId id : globalState_.conjuncts.keySet()) { > remove (pretty clear from function comment) Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@691 PS4, Line 691: * Returns true if this expr matches 'that'. Two exprs match if: > Move this into SlotRef since it's SlotRef specific? Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@696 PS4, Line 696: */ > Returns true if this expr matches 'that' Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@699 PS4, Line 699: if (this instanceof CastExpr && ((CastExpr)this).isImplicit()) { > For every pair of corresponding SlotRefs, slotRefCmp.matches() returns true Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@700 PS4, Line 700: return children_.get(0).matches(that, slotRefCmp); > localEquals() Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@719 PS4, Line 719: protected boolean localEquals(Expr that) { > I think we should separate matches() and localEquals() more cleanly. I thin Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@727 PS4, Line 727: */ > Not checking fn_ seems a little strange. The function semantics would be cl Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@962 PS4, Line 962: if (newExpr.matches(expr, cmp)) { > according to matches() using 'cmp' Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@198 PS4, Line 198: > A wrapper around localEquals(). Done http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@399 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. http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/util/Graph.java File fe/src/main/java/org/apache/impala/util/Graph.java: http://gerrit.cloudera.org:8080/#/c/8317/3/fe/src/main/java/org/apache/impala/util/Graph.java@52 PS3, Line 52: private final IntArrayList[] adjList_; > This addresses the performance issue, but it does not address the testing/r Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java File fe/src/main/java/org/apache/impala/util/Graph.java: http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java@91 PS4, Line 91: > @Override Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java@106 PS4, Line 106: /** > Don't understand the second sentence That sentence is removed. http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java@134 PS4, Line 134: visited.set(dstIt.peek()); > A graph condensed by its strongly-connected components (SCC). Vertices are Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java@152 PS4, Line 152: * their SCCs and an inner graph on the SCCs is stored. > @Override Done http://gerrit.cloudera.org:8080/#/c/8317/4/fe/src/main/java/org/apache/impala/util/Graph.java@199 PS4, Line 199: public int peek() { > Very clear now! Thanks. Ack -- To view, visit http://gerrit.cloudera.org:8080/8317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings 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 <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Wed, 15 Nov 2017 00:31:45 +0000 Gerrit-HasComments: Yes
