Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18327 )
Change subject: IMPALA-11123: Optimize count(star) for ORC scans ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/18327/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18327/1//COMMIT_MSG@16 PS1, Line 16: ACID table for full ACID tables http://gerrit.cloudera.org:8080/#/c/18327/1//COMMIT_MSG@18 PS1, Line 18: 'currentTransaction' column. If there is no delete file in an ACID table, then we should be able to still use the optimization, right? I am ok with not doing it in this change, but a ticket could be created to improve (full) ACID performance. My understanding is that FULL acid is probably the biggest reason why someone would use Impala with ORC. http://gerrit.cloudera.org:8080/#/c/18327/1/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/18327/1/be/src/exec/hdfs-orc-scanner.cc@786 PS1, Line 786: // We try to allocate a smaller row batch here because in most cases the number : // stripes in a file is much lower than the default row batch capacity. Why do we add one row per stripe? We could add them together and always add a single row. This would mean that this function will be only called once per split, so we can simply set eos_ to true before returning. (I also don't get why the Parquet scanner creates multiple rows) http://gerrit.cloudera.org:8080/#/c/18327/1/be/src/exec/hdfs-orc-scanner.cc@798 PS1, Line 798: assemble_rows_timer_.Start(); I think that starting the timer is not too useful here as this logic should take only minimal time. http://gerrit.cloudera.org:8080/#/c/18327/1/be/src/exec/hdfs-orc-scanner.cc@818 PS1, Line 818: uint64_t file_rows = reader_->getNumberOfRows(); : if (stripe_rows_read_ == file_rows) { We didn't change this logic, but it seems a bit suspicious to me: will these work if the ORC file has multiple HDFS block, so multiple scanners will process the same file? stripe_rows_read_ doesn't seem to be increased when we skip a stripe in NextStripe(), so my guess is that stripe_rows_read_ will never reach file_rows in multi block ORC files. http://gerrit.cloudera.org:8080/#/c/18327/1/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/18327/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@334 PS1, Line 334: isAcidTable I think that it should contain "full" to make the semantics clearer. + member variables should end with "_" http://gerrit.cloudera.org:8080/#/c/18327/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@405 PS1, Line 405: if (fileFormats.size() != 1) return false; Not sure if this worth the effort, but mixed ORC / Parquet tables could use count star optimization, right? http://gerrit.cloudera.org:8080/#/c/18327/1/testdata/workloads/functional-query/queries/QueryTest/orc-stats-agg.test File testdata/workloads/functional-query/queries/QueryTest/orc-stats-agg.test: http://gerrit.cloudera.org:8080/#/c/18327/1/testdata/workloads/functional-query/queries/QueryTest/orc-stats-agg.test@100 PS1, Line 100: ===== For Parquet we also have a multi block test: https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/parquet-stats-agg.test#L107 I don't know whether we have multiblock tables available for ORC, but it would be generally great to test those too with count(star) -- To view, visit http://gerrit.cloudera.org:8080/18327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fafa1182f97323aeb9ee39dd4e8ecd418fa6091 Gerrit-Change-Number: 18327 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Mar 2022 16:22:41 +0000 Gerrit-HasComments: Yes
