Daniel Becker 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. 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? 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 lists as parameters. 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? 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 lists as parameters. 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. 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? -- 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 12:35:09 +0000 Gerrit-HasComments: Yes
