Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12415 )

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


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema-test.cc
File src/kudu/common/schema-test.cc:

http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema-test.cc@153
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 
instead?


http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema-test.cc@173
PS3, Line 173:       NO_FATALS(copied_schema.ToString()); // NOLINT(*)
Doesn't seem like NO_FATALS has any purpose here (or on L185).


http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema.h
File src/kudu/common/schema.h:

http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema.h@302
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?


http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema.h@760
PS3, Line 760: const Schema &other
Nit: while you're here, "const Schema& other"


http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:

http://gerrit.cloudera.org:8080/#/c/12415/3/src/kudu/common/schema.cc@196
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 
latter:
  $ git grep '=\*/ ' src/ | wc -l
  170
  $ git grep '=\*/[^ ]' src/ | wc -l
  45



--
To view, visit http://gerrit.cloudera.org:8080/12415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Reply via email to