Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9645 )
Change subject: KUDU-2303: Add KuduSchema::ToString implementation ...................................................................... Patch Set 1: (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: // Test that KuduSchema::ToString returns the correct table info. I don't think this comment is needed- we get the same information from the name of the test. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5502 PS1, Line 5502: test on unique PK Comments should begin with a capital letter and end with a period. Usually. See https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5503 PS1, Line 5503: "primary key (key)\nSchema Hmm..I'm not a fan of this way of stringifying the schema. It looks something like primary key (my_key) Schema [ my_key[int32 NOT NULL] ] which is weird because the key is outside the "Schema [" part. I think it should look like Schema [ primary key (my_key1, my_key2), my_key1[int32 NOT NULL], my_key2[string NOT NULL], my_float[float NULLABLE] ] In fact I think Schema::ToString ought to act the same way-- the primary key is part of a schema and ought to be part of its string representation. Would you mind seeing if you could change Schema::ToString (i.e. from common/schema.h) to resemble the above? Then you should be able to just call into Schema::ToString to implement KuduSchema::ToString. You might need to change a few scattered tests that use Schema::ToString() too. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5503 PS1, Line 5503: [\n Can you lay out the expected strings so they resemble the output with newlines rendered? E.g. string schema_str_1 = "primary key (key)\n" "\tSchema [\n" "\tkey[int32 NOT NULL],\n" "\tint_val[int32 NOT NULL],\n" "\tstring_val[string NULLABLE],\n" "\tnon_null_with_default[int32 NOT NULL]\n" "]"; Recall that adjacent string literals are concatenated by the compiler. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5504 PS1, Line 5504: ASSERT_EQ ASSERT_EQ ends execution of the test if it fails. Since we'll have a few small, independent test cases, use EXPECT_EQ instead. EXPECT_EQ will fail the test and log a nice message if the equality test fails, but execution of the test continues. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5508 PS1, Line 5508: KuduSchema schema; Could you add a test case calling ToString() on an uninitialized KuduSchema to make sure it does something sensible? http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5519 PS1, Line 5519: CHECK_OK ASSERT_OK in tests. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/client-test.cc@5520 PS1, Line 5520: Extra whitespace. 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. See https://google.github.io/styleguide/cppguide.html#Spaces_vs._Tabs http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.h@568 PS1, Line 568: @return String with the first line specifying the key column names and : /// the rest of lines the stringified schema. I think this method is simple enough that a trivial @return doc suffices, e.g. @return A string describing this schema. 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: std:: Remove unneeded std:: namespace qualifier. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@741 PS1, Line 741: Likewise, 2 spaces instead of a tab. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@741 PS1, Line 741: vector<int> indexes; : GetPrimaryKeyColumnIndexes(&indexes); I think you could just iterate over the indexes directly, like in KuduSchema::GetPrimaryKeyColumnIndexes. http://gerrit.cloudera.org:8080/#/c/9645/1/src/kudu/client/schema.cc@745 PS1, Line 745: primary_key_info += Column(index).name() + ","; This is inefficient: every time a new column is added to the key, the entire primary_key_info string must be re-allocated. Or it might be, not sure how C++ will handle it internally. In any case, could you redo how the strings are joined so it's guaranteed not to be n^2? Look at Schema::ToString() for one method. The method there should also remove the fudge you had to do below to remove the last ',' and add the ')'. -- 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: 1 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 15 Mar 2018 06:19:01 +0000 Gerrit-HasComments: Yes
