Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/21483/3/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/21483/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@1044 PS3, Line 1044: if (this instanceof CastExpr && ((CastExpr)this).isImplicit()) { > It would be more OOP-like if CastExpr overrode this method, like SlotRef. I made this to match equals, but I think it was implemented to keep most of the SlotRef.Comparator logic in this file. Done. http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java File fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java: http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@49 PS3, Line 49: private List<Expr> lhs_; // left-hand side > Do we actually need these lists, can't we use a LinkedHashMap instead? Existing code manipulates LHS and RHS directly in a way that would be difficult to support with LinkedHashMap. I didn't want to do such a significant rewrite for this change, which this gets essentially the same performance improvement. http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@126 PS3, Line 126: result.lhs_.addAll(f.lhs_); : result.rhs_ = Expr.substituteList(f.rhs_, g, analyzer, false); : // Ensure map is available for the next step. : result.buildMap(); > We could use the constructor of ExprSubstitutionMap() that takes the two li Done http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@144 PS3, Line 144: Expr rhs = g.rhs_.get(i).clone(); > Couldn't we use put() here? Done http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@164 PS3, Line 164: result.lhs_.addAll(f.lhs_); : result.lhs_.addAll(g.lhs_); : result.rhs_.addAll(f.rhs_); : result.rhs_.addAll(g.rhs_); : : result.buildMap(); : result.verify(); > We could use the constructor of ExprSubstitutionMap() that takes the two li Done http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@199 PS3, Line 199: Checks that the lhs_ has no duplicates > I don't think we check that now. Done http://gerrit.cloudera.org:8080/#/c/21483/3/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@209 PS3, Line 209: Preconditions.checkState(substitutions_.get(lhs_.get(i)).equals(rhs_.get(i))); > We didn't check this before, could you give me some insight here? substitutions_ didn't exist before. This is primarily checking that our hashing is consistent with equals. If we do a lookup based on lhs, do we get the right rhs? -- To view, visit http://gerrit.cloudera.org:8080/21483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe Gerrit-Change-Number: 21483 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Fri, 07 Jun 2024 18:48:30 +0000 Gerrit-HasComments: Yes
