wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18829 )
Change subject: IMPALA-7942 (part 1): Add query hints for table cardinalities ...................................................................... Patch Set 4: (13 comments) Hi Quanlong, thanks for advice. I think you are right, use hint value to replace original table stats may cause consistency when use explain. So I modify the code, table hint is valid when no stats or has corrupt stats. Here is a problem, I use 'functional.alltypes' for hdfs table with stats, 'functional_parquet.alltypes' for hdfs table without stats, 'functional_kudu.alltypes' for kudu table with stats. But I did not figure out the way to test kudu table without stats. http://gerrit.cloudera.org:8080/#/c/18829/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18829/3//COMMIT_MSG@10 PS3, Line 10: query planning. > nit: generation? or "query planning", "query optimization" Done http://gerrit.cloudera.org:8080/#/c/18829/3//COMMIT_MSG@19 PS3, Line 19: l not : valid if table stat > nit: regardless the existense of the stats. Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@173 PS3, Line 173: // Value of query hint 'TABLE_NUM_ROWS' on this table. Used in constructing ScanNode if : // the table does not have stats, or has correct stats. -1 indicates no hint. Currently, : // this hint is valid for hd > nit: might be better to reword to Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@510 PS3, Line 510: > nit: isTableHintSupported Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@514 PS3, Line 514: estTable() != null && > nit: reword to Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@518 PS3, Line 518: for (PlanHint hint: tableHints_) { > Does this mean we support such hints for Kudu tables now? I think the SCHED Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@555 PS3, Line 555: analyzer.setHasPlanHints(); > nit: can we remove this comment? It seems no need to explain the following Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@556 PS3, Line 556: Long.parseLo > nit: can use Long.parseLong() directly, which is used internally in Long.va Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java@564 PS3, Line 564: Returns whether the table supports hint. Currently, > nit: reword to Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1465 PS3, Line 1465: cardinality_ = extrapolatedNumRows_; > Should we overwrite this as well if the hint exists? Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1542 PS3, Line 1542: * partitions with corrupt stats. > Could you please mention the hint in this comment? Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1573 PS3, Line 1573: // by each of the partitions, as the row count for the table. > I thought we only use the hint when missing stats. This always overwrites t Done http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java File fe/src/main/java/org/apache/impala/planner/ScanNode.java: http://gerrit.cloudera.org:8080/#/c/18829/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java@81 PS3, Line 81: // Refer to the comment of 'TableRef.tableNumRowsHint_' : protected long tableNumRowsHint_ = -1; : : public ScanNode(PlanNodeId id, TupleDes > nit: maybe we can just refer to the comment of TableRef.tableNumRowsHint_ Done -- To view, visit http://gerrit.cloudera.org:8080/18829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f0c773f4e67782a1428db64062f68afbd257af7 Gerrit-Change-Number: 18829 Gerrit-PatchSet: 4 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Fri, 19 Aug 2022 13:06:03 +0000 Gerrit-HasComments: Yes
