Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18050 )
Change subject: [WIP] IMPALA-10992 Planner changes for estimate peak memory - v2 ...................................................................... Patch Set 19: (7 comments) Start to address review comments. More to come! http://gerrit.cloudera.org:8080/#/c/18050/18/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/18050/18/common/thrift/Frontend.thrift@721 PS18, Line 721: query_size_threshold > what's the unit for query size? Should be in bytes. This data structure definition probably will be removed after integration. http://gerrit.cloudera.org:8080/#/c/18050/6/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/18050/6/common/thrift/ImpalaService.thrift@710 PS6, Line 710: // Indicates whether to relan. > typo relan Done http://gerrit.cloudera.org:8080/#/c/18050/6/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18050/6/common/thrift/Query.thrift@564 PS6, Line 564: // distributed plan among several executor groups. The returned plan satisfies > change "several" to "different-sized" Done http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@1192 PS6, Line 1192: * Substitutes the result expressions, the partition key expressions, and the primary > Why is cloning necessary? Per notes on P10, it looks like the expression list is modified during one iteration of replan. Hence the need to clone. explain insert into table functional.alltypes partition(year, month) select * from functional.alltypes; 59 Error Stack: 60 java.lang.IndexOutOfBoundsException: toIndex = 11 <=== resolved: via copy resultExpr in InsertStmt , which is an extra deep copied data members in InsertStmt.cstr(). It is not a good idea to share AnalysisResults_ in PlannerContext as the stuff is modified. 61 at java.util.ArrayList.subListRangeCheck(ArrayList.java:1014) 62 at java.util.ArrayList.subList(ArrayList.java:1006) 63 at org.apache.impala.planner.HdfsTableSink.collectExprs(HdfsTableSink.java:345) 64 at org.apache.impala.planner.Planner.createPlanFragments(Planner.java:283) 65 at org.apache.impala.planner.Planner.createPlans(Planner.java:303) 66 at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1562) 67 at org.apache.impala.service.Frontend.getPlannedExecRequest(Frontend.java:1939) 68 at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:1779) 69 at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:1644) 70 at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1614) 71 at org.apache.impala.planner.PlannerTestBase.testPlan(PlannerTestBase.java:513) 72 at org.apache.impala.planner.PlannerTestBase.runTestCase(PlannerTestBase.java:424) 73 at org.apache.impala.planner.PlannerTestBase.runPlannerTestFile(PlannerTestBase.java:909) 74 at org.apache.impala.planner.PlannerTestBase.runPlannerTestFile(PlannerTestBase.java:845) 75 at org.apache.impala.planner.PlannerTest.testInsertDefaultNoClusteredNoShuffle(PlannerTest.java:295) 76 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@1199 PS6, Line 1199: primaryKeyExprs_ = Expr.substituteList(primaryKeyExprs_, smap, analyzer, true); > Restore should be able to assign/clear rather than clone. All backup and restore methods are replaced with clone. http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@104 PS6, Line 104: // returns a single row. > rather than backup/restore, it may be a little cleaner to have an immutable Done http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/18050/6/fe/src/main/java/org/apache/impala/planner/Planner.java@359 PS6, Line 359: PrintUtils.printBytesRoundedToMb(request.getPer_host_mem_estimate()))); > please remove reformatting of otherwise unchanged code. Done -- To view, visit http://gerrit.cloudera.org:8080/18050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8a31a574b364f39b049a4bae33a8b98c5fc20bd Gerrit-Change-Number: 18050 Gerrit-PatchSet: 19 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 15 Dec 2021 19:02:39 +0000 Gerrit-HasComments: Yes
