David Ribeiro Alves has posted comments on this change. Change subject: Expose row format flags in KuduScanner ......................................................................
Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/client/client.cc File src/kudu/client/client.cc: PS8, Line 1247: formal > typo Done PS8, Line 1249: switch (flags) { : case NO_FLAGS: : case PAD_UNIXTIME_MICROS_TO_16_BYTES: : break; : default: : return Status::InvalidArgument(Substitute("Invalid row format flags: $0", flags)); : } > nit: maybe, put this 'more static' check prior to the 'if (data_->open_)' o Done http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/client/client.h File src/kudu/client/client.h: PS8, Line 2065: int64_t > btw, 1UL << 32 would not fit into the RowFormatFlags enum. It would be nec ah, good catch. changed these to static const fields http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: PS8, Line 306: int64_t > nit: if it's not changing during lifetime of the object, consider making ad This is changed on Reset() so it can't be const http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/client/schema.h File src/kudu/client/schema.h: Line 218: > nit: was this line added intentionally? Done http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/integration-tests/all_types-itest.cc File src/kudu/integration-tests/all_types-itest.cc: PS8, Line 98: > > > Nit: with C++11 we no longer need to separate triangular brackets with spac Done http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/tserver/scanners-test.cc File src/kudu/tserver/scanners-test.cc: PS8, Line 43: 0 > nit: why not to use the new shiny NO_FLAGS constant? Done http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: PS8, Line 333: int64_t > nit: since it is not changed during lifetime of the Scanner, maybe make it Done http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: PS8, Line 274: bitset in this int64 > If it's a bitset, then why not uint64? Unsigned types look like a natural we've been generally trying to avoid unsigned ints, but since this is not a counter or a number I thing you're right. I've changed this to unsigned -- To view, visit http://gerrit.cloudera.org:8080/6624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
