Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/18903 )
Change subject: KUDU-3379 Add column comments to table describe ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/18903/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18903/2//COMMIT_MSG@12 PS2, Line 12: > nit: unnecessary comma Done http://gerrit.cloudera.org:8080/#/c/18903/2//COMMIT_MSG@16 PS2, Line 16: se and' > nit: these are 'bitwise and' and 'bitwise or' operators Done http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/common/schema.h@278 PS2, Line 278: ToStr > I'm curious why const is needed here? I left it there by accident, it wasn't intended. Removed the const. http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/common/schema.cc@473 PS2, Line 473: |= ColumnSch > Wouldn't that be '|=' instead? Also I'm curious as well about it. Yes, it is "|=". Sadly "|=" does not work out of the box. Created an overload for "|=", and changed these lines to use that operator. http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/common/schema.cc@479 PS2, Line 479: s > nit: drop the extra parentheses Done http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18903/2/src/kudu/tools/kudu-admin-test.cc@2106 PS2, Line 2106: : // Build the schema : { : KuduSchema schema; : KuduSchemaBuilder builder; : builder.AddColumn("foo")->Type(KuduColumnSchema::INT32)->NotNull(); : builder.AddColumn("bar")->Type(KuduColumnSchema::INT32)->NotNull() : ->Comment("comment for bar"); : builder.SetPrimaryKey({"foo"}); : ASSERT_OK(builder.Build(&schema)); : : // Create the table : unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator()); : ASSERT_OK(table_creator->table_name(kTableName) : .schema(&schema) : .set_range_partition_columns({"foo"}) : .num_replicas(1) : .set_owner("alice") : .Create()); : } : : / > nit: move all this under the same sub-scope? Done -- 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: 5 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: Fri, 26 Aug 2022 11:40:13 +0000 Gerrit-HasComments: Yes
