You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Paul Rogers (Code Review)" <ge...@cloudera.org> on 2018/11/06 06:03:43 UTC
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11883
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
IMPALA-7808: Refactor Analyzer for easier debugging
Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.
A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.
To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.
This CR introduces the inner algorithm class, but leaves the original
code at the original indent level to make reviews easier. You should see
the same code, now broken into smaller functions, with a new "driver"
section that calls these new functions from where the code itself
previously resided.
Once a review is done of the code in this form, I can revise this CR
with the only change being to indent the methods one level, which can be
reviewed as a second step.
One block of code had to be moved below the inner class since it is not
part of the analysis process.
Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Testing: Reran all tests, which passed.
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 227 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11883/1
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11883/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/2/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@669
PS2, Line 669: List<Expr> substAggExprs = Expr.substituteList(aggExprs_, ndvSmap_, analyzer_, false);
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 20:42:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1289/ : 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/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 06:40:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 1:
(4 comments)
Some comments related to coding style in Impala, I like the change otherwise.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461: { return; }
We generally do not use braces in one line "if (cond) stmt;".
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@219
PS1, Line 219:
here and at many other functions: in Impala we generally do not add an empty line at the beginning of functions.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@526
PS1, Line 526:
nit: extra space
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 16:38:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11883
to look at the new patch set (#2).
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
IMPALA-7808: Refactor Analyzer for easier debugging
Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.
A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.
To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.
This CR introduces the inner algorithm class, but leaves the original
code at the original indent level to make reviews easier. You should see
the same code, now broken into smaller functions, with a new "driver"
section that calls these new functions from where the code itself
previously resided.
Once a review is done of the code in this form, I can revise this CR
with the only change being to indent the methods one level, which can be
reviewed as a second step.
One block of code had to be moved below the inner class since it is not
part of the analysis process.
Testing: No functional change, changes are purely structure.
Reran all tests, which passed.
Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 625 insertions(+), 558 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11883/2
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 1:
(8 comments)
thanks for the change and making it easier to review. mostly minor comments from my end.
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG@37
PS1, Line 37: Testing
pls move this testing section above the Change-Id
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@176
PS1, Line 176: private final Analyzer analyzer;
these members should be suffixed with "_"
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@186
PS1, Line 186: Note tem
note, you can always include such review-level comments as comments in gerrit. no need for source changes to communicate these things.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389
PS1, Line 389: protected
did some other class need this protected?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@617
PS1, Line 617: public
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@659
PS1, Line 659: public
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@757
PS1, Line 757: public void verifyAggregation() throws AnalysisException {
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@894
PS1, Line 894: }
just to remind myelf... end of inner class.
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 17:58:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1301/ : 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/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:01:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 5:
Added indentation, fixed lines that became too long.
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:10:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1300/ : 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/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
IMPALA-7808: Refactor Analyzer for easier debugging
Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.
A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.
To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.
For the most part, the existign code was simply split into functions
and indented. One block of code had to be moved below the inner class
since it is not part of the analysis process.
Testing: No functional change, changes are purely structure.
Reran all tests, which passed.
Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Reviewed-on: http://gerrit.cloudera.org:8080/11883
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/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 767 insertions(+), 688 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@219
PS3, Line 219: // When there is a LIMIT clause in conjunction with an ORDER BY, the ordering exprs
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@274
PS3, Line 274: "Star exprs only expand to scalar-typed columns because complex-typed exprs " +
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@326
PS3, Line 326: * Generates and registers !empty() predicates to filter out empty collections directly
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@341
PS3, Line 341: * are required so are checked for !empty.
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@347
PS3, Line 347: * TODO: In some cases, it is possible to generate !empty() predicates for a correlated
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@366
PS3, Line 366: if (analyzer_.isOuterJoined(ref.getResolvedPath().getRootDesc().getId())) continue;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@425
PS3, Line 425: throw new AnalysisException("'*' expression in select list requires FROM clause.");
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@665
PS3, Line 665: List<Expr> substAggExprs = Expr.substituteList(aggExprs_, ndvSmap_, analyzer_, false);
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@681
PS3, Line 681: // However the original COUNT(c) should have returned 0 instead of NULL in this case.
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@756
PS3, Line 756: if (!sortInfo_.getSortExprs().get(i).isBound(multiAggInfo_.getResultTupleId())) {
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/11883/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@860
PS3, Line 860: // If 'exprRewritten' is true, we have to compose the new smap with the existing one.
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 22:20:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 2:
(12 comments)
Thanks for the reviews. Addressed the comments.
If you are ready, I'll go ahead and shift the indent of the methods in the inner class, which will mark a zillion likes as changed. In its current form, you can more easily see the actual changes.
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG@37
PS1, Line 37: Reran a
> pls move this testing section above the Change-Id
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461: r
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461: eturn;
> We generally do not use braces in one line "if (cond) stmt;".
Thanks. Every project has its quirks; I 'm still learning Impala's preferences.
Fixed.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@176
PS1, Line 176: private final Analyzer analyzer_;
> these members should be suffixed with "_"
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@186
PS1, Line 186: Note tem
> note, you can always include such review-level comments as comments in gerr
Agreed. The comment is here to rather force the need to come back and shift indentation before the final +2. At that point, the comment will be removed.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@219
PS1, Line 219: if (multiAggInfo_ != null && multiAggInfo_.hasAggregateExprs()) {
> here and at many other functions: in Impala we generally do not add an empt
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389
PS1, Line 389: if (t
> did some other class need this protected?
Tried to minimize extra changes. It works to make this private, so went ahead and did so.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@526
PS1, Line 526: c
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@617
PS1, Line 617: su
> why public?
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@659
PS1, Line 659:
> why public?
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@757
PS1, Line 757: }
> why public?
Done
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@894
PS1, Line 894:
> just to remind myelf... end of inner class.
Yes. When you are satisfied that nothing changed other than the inner class, I'll go ahead and indent the whole mess.
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 20:44:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1295/ : 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/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 21:27:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11883
to look at the new patch set (#4).
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
IMPALA-7808: Refactor Analyzer for easier debugging
Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.
A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.
To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.
For the most part, the existign code was simply split into functions
and indented. One block of code had to be moved below the inner class
since it is not part of the analysis process.
Testing: No functional change, changes are purely structure.
Reran all tests, which passed.
Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 767 insertions(+), 688 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11883/4
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Nov 2018 02:56:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:03:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 2: Code-Review+2
go ahead and apply the correct indentation.
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 21:20:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11883
to look at the new patch set (#3).
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
IMPALA-7808: Refactor Analyzer for easier debugging
Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.
A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.
To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.
For the most part, the existign code was simply split into functions
and indented. One block of code had to be moved below the inner class
since it is not part of the analysis process.
Testing: No functional change, changes are purely structure.
Reran all tests, which passed.
Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 751 insertions(+), 688 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11883/3
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3434/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:15 +0000
Gerrit-HasComments: No