Adar Dembo 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) I reviewed the DeltaStore::may_have_deltas() piece and just scanned the rest. Could you add a unit test for it? Either deltamemstore-test or deltafile-test should work; probably the latter is better because you can test that reinserts show up as deltas. 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 shorten the names of the values. For example, ShortCircuitType::UNINITIALIZED, ShortCircuitType::SKIP_NONE, etc. 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'. 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? It might be more performant to add a bitmap variant of may_have_deltas_ where each bit represents a different column. Then this becomes something like: return may_have_deltas_ || may_have_deltas_per_col_[col_idx]; Regardless of how you handle it, you should rerun the microbenchmarks used in KUDU-2381 to ensure we don't regress that perf gain. 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. -- 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-Comment-Date: Fri, 02 Aug 2019 17:45:36 +0000 Gerrit-HasComments: Yes
