Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20922 )
Change subject: IMPALA-12726: Simulate large-scale query in TpcdsCpuCostPlannerTest ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/20922/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20922/2//COMMIT_MSG@24 PS2, Line 24: ar > nit: are Done http://gerrit.cloudera.org:8080/#/c/20922/2//COMMIT_MSG@28 PS2, Line 28: mult > nit: multi Done http://gerrit.cloudera.org:8080/#/c/20922/2/fe/src/main/java/org/apache/impala/catalog/SideloadTableStats.java File fe/src/main/java/org/apache/impala/catalog/SideloadTableStats.java: http://gerrit.cloudera.org:8080/#/c/20922/2/fe/src/main/java/org/apache/impala/catalog/SideloadTableStats.java@26 PS2, Line 26: > nit: Could you add description for the new class? Done http://gerrit.cloudera.org:8080/#/c/20922/2/fe/src/main/java/org/apache/impala/catalog/SideloadTableStats.java@40 PS2, Line 40: public SideloadTableStats(String tableName, long numRows, long totalSize) { > add Preconditions check for the input parameters Done http://gerrit.cloudera.org:8080/#/c/20922/3/fe/src/main/java/org/apache/impala/common/RuntimeEnv.java File fe/src/main/java/org/apache/impala/common/RuntimeEnv.java: http://gerrit.cloudera.org:8080/#/c/20922/3/fe/src/main/java/org/apache/impala/common/RuntimeEnv.java@21 PS3, Line 21: > nit: remove empty line The empty line in between is auto-formatted by clang-format. It looks like it carried by Chromium java style. https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/java/java.md#Import-Order I choose to remove StringUtils import instead. http://gerrit.cloudera.org:8080/#/c/20922/3/fe/src/test/java/org/apache/impala/testutil/StatsJsonParser.java File fe/src/test/java/org/apache/impala/testutil/StatsJsonParser.java: http://gerrit.cloudera.org:8080/#/c/20922/3/fe/src/test/java/org/apache/impala/testutil/StatsJsonParser.java@90 PS3, Line 90: colType.substring(0, colType.indexOf("(") > if colType contains "(", does it contains ")"? Yes. An example is "DECIMAL(7,2)". In that case, only "DECIMAL" is taken. Any invalid type input will be catch by default handler at line 153. -- To view, visit http://gerrit.cloudera.org:8080/20922 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaffddd70c2da8376ca6c40f65606bbac46c34de7 Gerrit-Change-Number: 20922 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 31 Jan 2024 22:44:06 +0000 Gerrit-HasComments: Yes
