Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14380 )
Change subject: [cfile] KUDU-2852 Push predicate evaluation for int type RLE decoder ...................................................................... Patch Set 5: (5 comments) Just passing through with some style comments. http://gerrit.cloudera.org:8080/#/c/14380/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14380/5//COMMIT_MSG@18 PS5, Line 18: Tests: Although this level of detail is welcome, we're not super rigid about specifying tests in the commit message given that the code review speaks for itself. Looking at what you've written here, I'd drop everything except the benchmark bit, and promote that into an (informal) paragraph after "Added a utility method...". http://gerrit.cloudera.org:8080/#/c/14380/5//COMMIT_MSG@24 PS5, Line 24: - large number of rows 100k to 1M with run lengths of 1k to 10k Perhaps clearer as "100k to 1M rows with run lengths ranging from 1k to 10k"? http://gerrit.cloudera.org:8080/#/c/14380/5//COMMIT_MSG@28 PS5, Line 28: materializing_iterator_decoder_eval We generally prepend '--' to gflag names to emphasize that they're gflags and not something else (like a standard variable name). http://gerrit.cloudera.org:8080/#/c/14380/5/src/kudu/common/rowblock.h File src/kudu/common/rowblock.h: http://gerrit.cloudera.org:8080/#/c/14380/5/src/kudu/common/rowblock.h@221 PS5, Line 221: false); Nit: this should be aligned with sel_vec->mutable_bitmap() from L220. http://gerrit.cloudera.org:8080/#/c/14380/5/src/kudu/tablet/all_types-scan-correctness-test.cc File src/kudu/tablet/all_types-scan-correctness-test.cc: http://gerrit.cloudera.org:8080/#/c/14380/5/src/kudu/tablet/all_types-scan-correctness-test.cc@228 PS5, Line 228: int upper_val) { Nit: should be aligned with "int nrows". See https://google.github.io/styleguide/cppguide.html#Function_Calls for details. -- To view, visit http://gerrit.cloudera.org:8080/14380 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e05775ec1301d3d0b0365a7704b8e962a20455e Gerrit-Change-Number: 14380 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 11 Oct 2019 05:10:17 +0000 Gerrit-HasComments: Yes
