[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Tested-by: Impala Public Jenkins --- 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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 02:56:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:04:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:03:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:01:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 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
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 06 Nov 2018 22:20:29 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 21:27:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 21:20:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 06 Nov 2018 20:44:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 20:42:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 06 Nov 2018 17:58:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 06 Nov 2018 16:38:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 06 Nov 2018 06:40:36 + Gerrit-HasComments: No
[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
