Adar Dembo has posted comments on this change. ( )

Change subject: Schema copy and move constructors/operators must copy 

Patch Set 3:

File src/kudu/common/
PS3, Line 153:   // Test both with and without column ids.
             :   vector<Schema> schemas = { Schema(cols, 1), Schema(cols, ids, 
1) };
             :   for (const auto& schema : schemas) {
Since these two are independent of one another, could you parameterize the test 
PS3, Line 173:       NO_FATALS(copied_schema.ToString()); // NOLINT(*)
Doesn't seem like NO_FATALS has any purpose here (or on L185).
File src/kudu/common/schema.h:
PS3, Line 302:   // compare types in Equals function
Only somewhat related, but could you name this enum to make it consistent with 
the new Schema::Equals treatment?
PS3, Line 760: const Schema &other
Nit: while you're here, "const Schema& other"
File src/kudu/common/
PS3, Line 196:       name_to_index_(/*bucket_count*/ 10,
Nit: stylistically, I don't think this change brings us closer to style 
adherence. From a sampling of "git grep '[a-z]\*/' src/", I see this style used 
primarily to 'hide' function arguments that aren't used.

As for '/*foo=*/f' vs. '/*foo=*/ f', we seem to favor the former over the 
  $ git grep '=\*/ ' src/ | wc -l
  $ git grep '=\*/[^ ]' src/ | wc -l

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b8df1142dc733c446a5717ca8cae1309f3e867
Gerrit-Change-Number: 12415
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <>
Gerrit-Comment-Date: Wed, 13 Feb 2019 04:29:48 +0000
Gerrit-HasComments: Yes

Reply via email to