helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13987 )
Change subject: KUDU-2854 short circuit predicates on dictionary-coded columns ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/cfile/cfile_reader.h@236 PS1, Line 236: enum ShortCircuitType { > Nit: use an enum class here. The full qualification will allow you to short Done http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/delta_store.h@305 PS1, Line 305: // Returns true if there might exist deltas to be applied. It is safe to : // conservatively return true, but this would force a skip over decoder-level : // evaluation. > Please update to describe the effect of 'col_idx'. Done http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/delta_store.cc@494 PS1, Line 494: return may_have_deltas_ ? : (!updates_by_col_[col_idx].empty() || !deleted_.empty() || !reinserted_.empty()) : false; > Does this regress the perf gain from KUDU-2381? Could you check? Hmmm, the code here is close to O(1). It is different with KUDU-2381 who has 280 loops. If we use a bitmap variable((280 + 7) / 8 = 35 bytes), we have to clear(memset) the bitmap(35 loops?) every time there are deltas. In addition, It seems it's a little bit difficult to rerun the microbenchmarks you mentioned since the workload is not published except 280 columns. http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/13987/1/src/kudu/tablet/tablet-test-util.h@336 PS1, Line 336: DCHECK_EQ(row_slice.size(), schema.byte_size() + ContiguousRowHelper::null_bitmap_size(schema)); > This was already merged in d5eb68327; please rebase. Done -- To view, visit http://gerrit.cloudera.org:8080/13987 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id348583cc7d85773e8f32a189f4344d7a36a30b6 Gerrit-Change-Number: 13987 Gerrit-PatchSet: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 08 Aug 2019 09:59:53 +0000 Gerrit-HasComments: Yes
