wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18061 )
Change subject: IMPALA-11040: Remove unnecessary reset() method in class 'UnionStmt' ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/18061/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18061/2//COMMIT_MSG@17 PS2, Line 17: cause deep nesting. > nit: cause deep nesting. Done http://gerrit.cloudera.org:8080/#/c/18061/2//COMMIT_MSG@22 PS2, Line 22: would execute slowly, or even fail. > nit: would execute slowly, or even fail. Done http://gerrit.cloudera.org:8080/#/c/18061/2/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java: http://gerrit.cloudera.org:8080/#/c/18061/2/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@a67 PS2, Line 67: > I think we probably want this to be executed, we just don't want it to be e Hi Zoltan, thanks for your patient review, but I don't understand. Do you mean like this? @Override public void reset() { super.reset(); if (resetWasInvoked_) return; //...... } Here are some things that confuse me: 1.Why 'we probably want this to be executed'? I aggree that maybe we should keep 'UnionStmt.reset()' like this for future needs: @Override public void reset() { super.reset(); } But all codes after 'super.reset();' are included in 'SetOperationStmt.reset()', and these memebers are exists in 'SetOperationStmt' which are not been inherited by other sub-class, which means we do the same reset twice in 'UnionStmt.reset()' and 'SetOperationStmt.reset()'. 2.You mentioned that set 'resetWasInvoked_' as false in 'analyze()', when do we set as true? default value as true? In 'reAnalyze()' function, Impala first execute 'reset()', then 'analyze()'. If we set false in 'analyze()'. Before 'reAnalyze()' phase, Impala already execute 'analyze()', 'resetWasInvoked_' was set as false, and this check invalid. Hope for your reply again! http://gerrit.cloudera.org:8080/#/c/18061/2/testdata/workloads/functional-query/queries/QueryTest/union.test File testdata/workloads/functional-query/queries/QueryTest/union.test: http://gerrit.cloudera.org:8080/#/c/18061/2/testdata/workloads/functional-query/queries/QueryTest/union.test@1187 PS2, Line 1187: would execute slowly, or even fai > nit: would execute slowly, or even fail. Done -- To view, visit http://gerrit.cloudera.org:8080/18061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I408a396d40d9622f2ae6c459f49cbfcc19affd14 Gerrit-Change-Number: 18061 Gerrit-PatchSet: 3 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Wed, 08 Dec 2021 09:58:17 +0000 Gerrit-HasComments: Yes
