Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20498 )
Change subject: IMPALA-12018: Consider runtime filter for cardinality reduction ...................................................................... Patch Set 12: (16 comments) http://gerrit.cloudera.org:8080/#/c/20498/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20498/12//COMMIT_MSG@31 PS12, Line 31: mode Nit: modes. http://gerrit.cloudera.org:8080/#/c/20498/12//COMMIT_MSG@35 PS12, Line 35: RUNTIME_FILTER_CARDINALITY_REDUCTION_SCALE Could you elaborate on how it can be used (what values are valid, what they mean) and what the default value is? http://gerrit.cloudera.org:8080/#/c/20498/12/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/20498/12/be/src/service/query-options-test.cc@315 PS12, Line 315: TQueryOptions options; I'd put it right before the loop. http://gerrit.cloudera.org:8080/#/c/20498/12/be/src/service/query-options-test.cc@316 PS12, Line 316: QueryConstants qc; This is not used. http://gerrit.cloudera.org:8080/#/c/20498/12/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/20498/12/be/src/service/query-options.cc@1170 PS12, Line 1170: f No need for the 'float' literal, especially as it is a double. http://gerrit.cloudera.org:8080/#/c/20498/12/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/20498/12/common/thrift/ImpalaService.thrift@837 PS12, Line 837: exceed Nit: exceeds. http://gerrit.cloudera.org:8080/#/c/20498/12/common/thrift/ImpalaService.thrift@849 PS12, Line 849: Default Nit: Defaults. http://gerrit.cloudera.org:8080/#/c/20498/12/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/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@474 PS12, Line 474: only applies at HDFS columnar file Now 'isAllColumnarScanner' is replaced with 'evalAtRowLevel', Kudu is also included, isn't it? http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@485 PS12, Line 485: lower Maybe 'reduced' would be better, "lower o. c. estimate" may suggest a "lower estimate", i.e. a lower bound. http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@486 PS12, Line 486: in Nit: is? http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@488 PS12, Line 488: below It can be a bit confusing that this join node is at the bottom of the stack but there are nodes below it. I know other nodes are below it in the node tree, not in the stack, but this could be made explicit. http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@561 PS12, Line 561: join nodes Can it be multiple join nodes or just one? http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@565 PS12, Line 565: * the simplest join cardinality formula from JoinNode.computeGenericJoinCardinality(). Could you also mention 'reductionScale' here? http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@578 PS12, Line 578: nodeStack.get(i).getCardinality() Could extract this as 'currentCardinality'. http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@591 PS12, Line 591: scanRangeSelectivity_ Shouldn't it be 'partitionSelectivity'? Or is it the same thing? http://gerrit.cloudera.org:8080/#/c/20498/12/fe/src/main/java/org/apache/impala/planner/ScanNode.java@609 PS12, Line 609: > 1 Why do we leave out the last one? -- To view, visit http://gerrit.cloudera.org:8080/20498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I033789c9b63a8188484e3afde8e646563918b3e1 Gerrit-Change-Number: 20498 Gerrit-PatchSet: 12 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2023 10:18:40 +0000 Gerrit-HasComments: Yes
