Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14347 )
Change subject: IMPALA-6501: Optimize count(star) for Kudu scans ...................................................................... Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/14347/10/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: http://gerrit.cloudera.org:8080/#/c/14347/10/be/src/exec/kudu-scanner.cc@110 PS10, Line 110: Tuple* tuple = reinterpret_cast<Tuple*>(tuple_buffer); : Tuple::ClearNullBits(tuple, scan_node_->tuple_desc()->null_bytes_offset(), : scan_node_->tuple_desc()->num_null_bytes()); : int64_t* counter_slot = tuple->GetBigIntSlot(scan_node_->count_star_slot_offset()); : *counter_slot = 0; optional: I think that this code would be even clearer by doing the loop first (counting in a local variable), and then do all the tuple and row batch stuff in a separate block. http://gerrit.cloudera.org:8080/#/c/14347/10/be/src/exec/kudu-scanner.cc@130 PS10, Line 130: tuple = next_tuple(tuple); This seems unnecessary. http://gerrit.cloudera.org:8080/#/c/14347/10/be/src/exec/kudu-scanner.cc@132 PS10, Line 132: if (*eos) *eos must be true at this point, so I would replace this with a DCHECK http://gerrit.cloudera.org:8080/#/c/14347/10/be/src/exec/kudu-scanner.cc@140 PS10, Line 140: RETURN_IF_ERROR( : row_batch->ResizeAndAllocateTupleBuffer(state_, &tuple_buffer_size, &tuple_buffer)); possible further optimization: It would be enough to allocate a buffer for a single tuple, as now we are only adding a single row to the row batch in the count(*) case. ResizeAndAllocateTupleBuffer has an overload which takes an additional parameter for capacity: https://github.com/apache/impala/blob/master/be/src/runtime/row-batch.cc#L560 -- To view, visit http://gerrit.cloudera.org:8080/14347 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic99e0f954d0ca65779bd531ca79ace1fcb066fb9 Gerrit-Change-Number: 14347 Gerrit-PatchSet: 10 Gerrit-Owner: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Norbert Luksa <norbert.lu...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Oct 2019 14:16:29 +0000 Gerrit-HasComments: Yes
