Dan Burkert has posted comments on this change. Change subject: Integrate ColumnPredicate into client and server ......................................................................
Patch Set 13: (8 comments) http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 282: scan->clear_deprecated_range_predicates(); > do you ever set them anymore in the current code? no need to clear if not Done http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/common/key_util.cc File src/kudu/common/key_util.cc: Line 88: // Increments a key with the provided column indices to the smalles key which is > typo: smallest Done Line 193: for (auto col_idx = first; col_idx < last; std::advance(col_idx, 1)) { > why not col_idx++ ? advance is more general in that it will work with iterators that aren't random access iterators. I think it's the case that we only use this with random access iterators, though. rename done. http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/common/key_util.h File src/kudu/common/key_util.h: Line 38: // than the current key. See IncrementKey for details. > hmm, IncrementKey() isn't defined in this header, though. I think keeping t Done http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/common/scan_spec-test.cc File src/kudu/common/scan_spec-test.cc: Line 83: // Set the exclusive lower bound of the spec to the provided row. The row must > typo: upper Done http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/common/scan_spec.cc File src/kudu/common/scan_spec.cc: Line 158: break; > do you need this break? wouldn't the for loop end anyway here? No, consecutive lower bound constraints are pushed into the primary key, but after the first they should not be removed from the predicate set. If you want to see this in action you can remove the break and look at the test failures. Line 179: if (lower_bound_key_ != nullptr || exclusive_upper_bound_key_ != nullptr) { > can you invert thsi condition to just return early if there is no bound? Done http://gerrit.cloudera.org:8080/#/c/2138/13/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1262: && !ContainsKey(missing_col_names, column)) { > nit: && on prev line Done -- To view, visit http://gerrit.cloudera.org:8080/2138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ife6852680b7f59fddee688e5702c1a70944f7622 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
