Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/9645 )
Change subject: KUDU-2303: Add KuduSchema::ToString implementation ...................................................................... Patch Set 5: (14 comments) http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5500 PS1, Line 5500: } > I don't think this comment is needed- we get the same information from the Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5502 PS1, Line 5502: > Comments should begin with a capital letter and end with a period. Usually. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5503 PS1, Line 5503: > Can you lay out the expected strings so they resemble the output with newli Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5503 PS1, Line 5503: > Hmm..I'm not a fan of this way of stringifying the schema. It looks somethi Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5504 PS1, Line 5504: / Test th > ASSERT_EQ ends execution of the test if it fails. Since we'll have a few sm Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5508 PS1, Line 5508: FLAGS_user_acl = " > Could you add a test case calling ToString() on an uninitialized KuduSchema Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5519 PS1, Line 5519: KuduClie > ASSERT_OK in tests. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5520 PS1, Line 5520: > Extra whitespace. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.h@566 PS1, Line 566: > Indent with spaces instead of tabs, 2 spaces per level of indentation. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.h@568 PS1, Line 568: @return A string describing this schema. : std::string ToString() const; > I think this method is simple enough that a trivial @return doc suffices, e Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@740 PS1, Line 740: strin > Remove unneeded std:: namespace qualifier. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@741 PS1, Line 741: > Likewise, 2 spaces instead of a tab. Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@741 PS1, Line 741: return schema_ ? schema_->ToString() : ""; : } > I think you could just iterate over the indexes directly, like in KuduSchem Done http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@745 PS1, Line 745: // namespace kudu > This is inefficient: every time a new column is added to the key, the entir Done -- To view, visit http://gerrit.cloudera.org:8080/9645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43958c3733bf273c810cc77068e8d69a3fb2f132 Gerrit-Change-Number: 9645 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 16 Mar 2018 16:43:52 +0000 Gerrit-HasComments: Yes
