You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2024/06/06 18:09:18 UTC
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21483
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Adds a HashMap to ExprSubstitutionMap to speed lookups while retaining
lists for correct ordering (ordering needs to match to SlotRef order).
Ignores duplicate inserts, preserving the old behavior that only the
first match would actually be usable; duplicates primarily show up as a
result of combining duplicate distinct and aggregate expressions, or
redundant nested aggregation (like the tests for IMPALA-10182).
Implements localHash and hashCode for Expr and related classes.
Avoids deep-cloning LHS Exprs in ExprSubstitutionMap as they're used for
lookup and not expected to be mutated.
Adds the many expressions test, which now runs in a handful of seconds.
Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/DateLiteral.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ValidTupleIdExpr.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
27 files changed, 1,681 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/21483/1
--
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: newchange
Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Gerrit-Change-Number: 21483
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
> I recently add those fields in IMPALA-8042 (https://gerrit.cloudera.org/c/2
Do you want to put a patch up to fix them?
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 22:26:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16279/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 18:34:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16299/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 19:12:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 03:14:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21483
to look at the new patch set (#3).
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Adds a HashMap to ExprSubstitutionMap to speed lookups while retaining
lists for correct ordering (ordering needs to match to SlotRef order).
Ignores duplicate inserts, preserving the old behavior that only the
first match would actually be usable; duplicates primarily show up as a
result of combining duplicate distinct and aggregate expressions, or
redundant nested aggregation (like the tests for IMPALA-10182).
Implements localHash and hashCode for Expr and related classes.
Avoids deep-cloning LHS Exprs in ExprSubstitutionMap as they're used for
lookup and not expected to be mutated.
Adds the many expressions test, which now runs in a handful of seconds.
Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/DateLiteral.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ValidTupleIdExpr.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
27 files changed, 1,693 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/21483/3
--
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: newpatchset
Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Gerrit-Change-Number: 21483
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
> That is actually better for backporting. I'll push fix for that later today
On second thought, this is not an issue. If betweenExprId_ is considered for equality, Expr.removeDuplicates() will not work as intended.
https://gerrit.cloudera.org/c/21377/10/fe/src/main/java/org/apache/impala/analysis/Analyzer.java#2638
Please ignore this comment.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
File fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java@123
PS2, Line 123: Objects.hash(super.localHash(), op_);
> I should include them into localEquals(), for IMPALA-8042, but I forgot.
This is not an issue. Please ignore this comment.
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 23:34:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21483
to look at the new patch set (#2).
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Adds a HashMap to ExprSubstitutionMap to speed lookups while retaining
lists for correct ordering (ordering needs to match to SlotRef order).
Ignores duplicate inserts, preserving the old behavior that only the
first match would actually be usable; duplicates primarily show up as a
result of combining duplicate distinct and aggregate expressions, or
redundant nested aggregation (like the tests for IMPALA-10182).
Implements localHash and hashCode for Expr and related classes.
Avoids deep-cloning LHS Exprs in ExprSubstitutionMap as they're used for
lookup and not expected to be mutated.
Adds the many expressions test, which now runs in a handful of seconds.
Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/DateLiteral.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ValidTupleIdExpr.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
27 files changed, 1,694 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/21483/2
--
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: newpatchset
Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Gerrit-Change-Number: 21483
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 5: Code-Review+2
--
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: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Jun 2024 22:13:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 18:48:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21483
to look at the new patch set (#4).
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Adds a HashMap to ExprSubstitutionMap to speed lookups while retaining
lists for correct ordering (ordering needs to match to SlotRef order).
Ignores duplicate inserts, preserving the old behavior that only the
first match would actually be usable; duplicates primarily show up as a
result of combining duplicate distinct and aggregate expressions, or
redundant nested aggregation (like the tests for IMPALA-10182).
Implements localHash and hashCode for Expr and related classes.
Avoids deep-cloning LHS Exprs in ExprSubstitutionMap as they're used for
lookup and not expected to be mutated.
Adds the many expressions test, which now runs in a handful of seconds.
Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/DateLiteral.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ValidTupleIdExpr.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
27 files changed, 1,690 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/21483/4
--
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: newpatchset
Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Gerrit-Change-Number: 21483
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 12:35:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Adds a HashMap to ExprSubstitutionMap to speed lookups while retaining
lists for correct ordering (ordering needs to match to SlotRef order).
Ignores duplicate inserts, preserving the old behavior that only the
first match would actually be usable; duplicates primarily show up as a
result of combining duplicate distinct and aggregate expressions, or
redundant nested aggregation (like the tests for IMPALA-10182).
Implements localHash and hashCode for Expr and related classes.
Avoids deep-cloning LHS Exprs in ExprSubstitutionMap as they're used for
lookup and not expected to be mutated.
Adds the many expressions test, which now runs in a handful of seconds.
Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Reviewed-on: http://gerrit.cloudera.org:8080/21483
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/DateLiteral.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ValidTupleIdExpr.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
27 files changed, 1,690 insertions(+), 86 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: Ic538a82c69ee1dd76981fbacf95289c9d00ea9fe
Gerrit-Change-Number: 21483
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10704/ DRY_RUN=false
--
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: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Jun 2024 22:13:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
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:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
> I think this should also include isInferred_ and betweenExprId_
Why? Those aren't included in localEquals.
hashCode can allow duplicates, it distinguishes them with equals. But if two objects that would satisfy equals end up in different hash buckets, the hash table doesn't work.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java@204
PS2, Line 204: isDecode()
> Should this be decodeExpr_?
No, needs to mirror localEquals.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
File fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java@123
PS2, Line 123: Objects.hash(super.localHash(), op_);
> Include betweenSelectivity_?
No, needs to mirror localEquals.
http://gerrit.cloudera.org:8080/#/c/21483/1/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/1/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@48
PS1, Line 48: HashMap
> Question: is 2 List + 1 HashMap still faster for expression substitution ra
Using ArrayLists the way we access them likely is faster than LinkedHashMap. Not as sure of this in Java vs C++, but I wouldn't expect it to be slower.
http://gerrit.cloudera.org:8080/#/c/21483/1/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@204
PS1, Line 204: Preconditions.checkState(substitutions_.size() == lh
> Move this after L207? The Preconditions are probably light enough to evalua
Done
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
File fe/src/main/java/org/apache/impala/analysis/OrderByElement.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java@105
PS2, Line 105: public int hashCode() {
: return Objects.hash(getClass(), nullsFirstParam_, expr_, isAsc_);
: }
> Is expr_ included here because it it not child expression?
It's meant to capture all the objects required to be equal in "equals".
http://gerrit.cloudera.org:8080/#/c/21483/2/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/21483/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@358
PS2, Line 358: if (label_ != null) return label_.toLowerCase().hashCode();
: return super.localHash();
> I think rawPath_ is important to hash for nested type column?
Hmm, I'm not sure. As described in https://docs.oracle.com/javase%2F8%2Fdocs%2Fapi%2F%2F/java/lang/Object.html#hashCode--
- If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
- It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results.
That suggests that using rawPath_ in hashCode and label_ in equals may have only circumstantially worked. I think a dummy SlotRef could have both desc_ and rawPath_ be null, while label_ isn't.
The equals implementation is weird here, in that if desc_ is not null only desc_ has to match, which is why the hashCode had to be written this way rather than a hash of all relevant objects.
http://gerrit.cloudera.org:8080/#/c/21483/2/testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
File testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test:
http://gerrit.cloudera.org:8080/#/c/21483/2/testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test@4
PS2, Line 4:
> Replace tabs with spaces?
Sure. I just copied over the original file Joe uploaded.
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 21:32:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+2
Thanks Michael.
--
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: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Jun 2024 14:09:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
I think this should also include isInferred_ and betweenExprId_
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java@204
PS2, Line 204: isDecode()
Should this be decodeExpr_?
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
File fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java@123
PS2, Line 123: Objects.hash(super.localHash(), op_);
Include betweenSelectivity_?
http://gerrit.cloudera.org:8080/#/c/21483/1/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/1/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@48
PS1, Line 48: HashMap
Question: is 2 List + 1 HashMap still faster for expression substitution rather than using just 1 LinkedHashMap?
I'm guessing yes.
http://gerrit.cloudera.org:8080/#/c/21483/1/fe/src/main/java/org/apache/impala/analysis/ExprSubstitutionMap.java@204
PS1, Line 204: if (BackendConfig.INSTANCE.isReleaseBuild()) return;
Move this after L207? The Preconditions are probably light enough to evaluate multiple times.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@257
PS2, Line 257: params_.isDistinct(), params_.isIgnoreNulls(), params_.isStar()
Maybe define this as FunctionParams.localHash() ?
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
File fe/src/main/java/org/apache/impala/analysis/OrderByElement.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java@105
PS2, Line 105: public int hashCode() {
: return Objects.hash(getClass(), nullsFirstParam_, expr_, isAsc_);
: }
Is expr_ included here because it it not child expression?
http://gerrit.cloudera.org:8080/#/c/21483/2/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/21483/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@358
PS2, Line 358: if (label_ != null) return label_.toLowerCase().hashCode();
: return super.localHash();
I think rawPath_ is important to hash for nested type column?
http://gerrit.cloudera.org:8080/#/c/21483/2/testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test
File testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test:
http://gerrit.cloudera.org:8080/#/c/21483/2/testdata/workloads/functional-planner/queries/PlannerTest/many-expression.test@4
PS2, Line 4:
Replace tabs with spaces?
--
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: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 19:33:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
> Why? Those aren't included in localEquals.
I recently add those fields in IMPALA-8042 (https://gerrit.cloudera.org/c/21377/). I should include them into localEquals(), but I forgot. The same also for CompoundPredicate.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java@204
PS2, Line 204: isDecode()
> No, needs to mirror localEquals.
Ack
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
File fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java@123
PS2, Line 123: Objects.hash(super.localHash(), op_);
> No, needs to mirror localEquals.
I should include them into localEquals(), for IMPALA-8042, but I forgot.
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java
File fe/src/main/java/org/apache/impala/analysis/OrderByElement.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/OrderByElement.java@105
PS2, Line 105: public int hashCode() {
: return Objects.hash(getClass(), nullsFirstParam_, expr_, isAsc_);
: }
> It's meant to capture all the objects required to be equal in "equals".
Ack
http://gerrit.cloudera.org:8080/#/c/21483/2/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/21483/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@358
PS2, Line 358: if (label_ != null) return label_.toLowerCase().hashCode();
: return super.localHash();
> Hmm, I'm not sure. As described in https://docs.oracle.com/javase%2F8%2Fdoc
OK. I see that label_ is derived from rawPath_ in one of this class constructor. So the hashing over label_ should cover for rawPath_ as well.
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 22:17:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16285/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 21:58:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 4: Code-Review+1
--
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: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 21:31:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
http://gerrit.cloudera.org:8080/#/c/21483/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@428
PS2, Line 428: Objects.hash(super.localHash(), op_);
> Do you want to put a patch up to fix them?
That is actually better for backporting. I'll push fix for that later today.
--
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 <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 22:47:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
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: // HashMap ourselves.
> Existing code manipulates LHS and RHS directly in a way that would be diffi
substituteLhs I think would be hard to match the performance of this implementation with LinkedHashMap because we would essentially need to recreate the HashMap and linked list of LinkedHashMap, where-as this implementation only needs to rebuild the HashMap. Admittedly I haven't tested the performance implications, but I think replacing with LinkedHashMap can be considered a separate refactoring that requires additive work.
--
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: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Jun 2024 20:39:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21483 )
Change subject: IMPALA-12800: Use HashMap for ExprSubstitutionMap lookups
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16280/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Jun 2024 18:53:46 +0000
Gerrit-HasComments: No