Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/20477 )
Change subject: IMPALA-12313: (part 1) Refactor modify statements ...................................................................... Patch Set 2: (2 comments) Nice refactoring, thank you! There is only one thing I would like to add. IMO 'sourceStmt_' is more descriptive, and its type (QueryStmt) is visible in the IDE anyway. I would keep it. However, if you would like to rename 'sourceStmt_' to 'queryStmt_', please replace all the references in the comments as well to make it consistent. http://gerrit.cloudera.org:8080/#/c/20477/2/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java File fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java: http://gerrit.cloudera.org:8080/#/c/20477/2/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@60 PS2, Line 60: Result of the analysis of the internal SelectStmt that produces the rows that : // will be modified. In the earlier patch set, 'queryStmt_' used to be 'sourceStmt_'. A lot of comments and function names still refer to this variable as 'sourceStmt_'. I think you could mention in this comment that this query statement "serves as the source" or "is considered the source statement" or something like that. It helps the reader link the two names to the same variable. Semantically you already explained it in this comment, but it would be easier to understand and search for. http://gerrit.cloudera.org:8080/#/c/20477/2/fe/src/main/java/org/apache/impala/analysis/ModifyImpl.java@99 PS2, Line 99: sourceStmt_ nit: queryStmt_ -- To view, visit http://gerrit.cloudera.org:8080/20477 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If15f64944f2e23064b7112ad5930abc775dd65ec Gerrit-Change-Number: 20477 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Sep 2023 12:29:00 +0000 Gerrit-HasComments: Yes
