Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18903 )
Change subject: KUDU-3379 Add column comments to table describe ...................................................................... Patch Set 6: Code-Review+1 (6 comments) http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/common/schema.h@280 PS6, Line 280: static_cast<std::underlying_type_t<ToStringMode>>(lhs) | : static_cast<std::underlying_type_t<ToStringMode>>(rhs) If doing so much bit-wise fun, maybe drop 'class' and have just 'enum ToStringMode : uint8_t', so it's not necessary to cast 'lhs' and 'rhs' to use bit-wise operator on them? Otherwise, what does that 'class' buy you? Also, with non-class enum in C++17 it's possible to use both Schema::ToStringMode::BASE_INFO and Schema::BASE_INFO, IIRC. http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/common/schema.h@290 PS6, Line 290: static_cast<std::underlying_type_t<ToStringMode>>(lhs) & : static_cast<std::underlying_type_t<ToStringMode>>(rhs) ditto http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/common/schema.h@828 PS6, Line 828: static_cast<std::underlying_type_t<ToStringMode>>(lhs) | : static_cast<std::underlying_type_t<ToStringMode>>(rhs) ditto http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/common/schema.h@837 PS6, Line 837: static_cast<std::underlying_type_t<ToStringMode>>(lhs) : & static_cast<std::underlying_type_t<ToStringMode>>(rhs)); ditto http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/tools/kudu-admin-test.cc@1930 PS6, Line 1930: "-show_attributes=true", : "-show_column_comment" nit: please follow the same approach for both parameters here -- you either drop '=true' for '--show_attributes' or add '=true' for '--show_column_comment' http://gerrit.cloudera.org:8080/#/c/18903/6/src/kudu/tools/kudu-admin-test.cc@2100 PS6, Line 2100: // Simple tests to check whether the column describing flags : // work, alone and together as well. nit: move this to be outside of the scope, to precede the corresponding TEST_F -- To view, visit http://gerrit.cloudera.org:8080/18903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8b751bd821a5f50490caa6f1aaf1fc767de0222 Gerrit-Change-Number: 18903 Gerrit-PatchSet: 6 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 30 Aug 2022 02:34:37 +0000 Gerrit-HasComments: Yes
