Alexey Serbin has posted comments on this change. Change subject: Expose row format flags in KuduScanner ......................................................................
Patch Set 8: (8 comments) Just skimmed over the code to get more context on the new feature. http://gerrit.cloudera.org:8080/#/c/6624/8/src/kudu/client/client.cc File src/kudu/client/client.cc: PS8, Line 1247: formal typo 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_)' one? 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 necessary to switch to C++11 enums style like 'enum X : uint64_t' to accommodate those values or use some compiler flags, which might break ABI compatibility. Why to have this parameter of 8 byte type then? 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 adding 'const' modifier. 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? 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? 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 constant? 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 choice for bitset, especially if using the highest bit as well. It also helps if using std::bitset in the C++ code. -- 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
