honeyhexin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line ...................................................................... Patch Set 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@260 PS7, Line 260: Gets the vector containin > nit: reword this as "Returns a vector containing the column indexes of the Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@263 PS7, Line 263: Status GetRangeSchemaColumnIndexes(const Schema& schema, : std::vector<int>* range_column_idxs) const; : > Is this used anywhere? This function is useless now. I have delete it. http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc@1245 PS7, Line 1245: if (idx == Schema::kColumnNotFound) { > Do we need to worry about what happens if the column ID isn't in the schema I have add the process in the newest patch: if any of the columns is not in the key range columns then an InvalidArgument status is returned. http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2346 PS7, Line 2346: TEST_F(AdminCliTest, TestAddAndDropUnboundedPartition) { > Could you split this test into multiple test cases that test the following: Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2354 PS7, Line 2354: > nit: maybe call this kTestTableName so it's clear this is a table name inst Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2370 PS7, Line 2370: turn Status::OK(); : }; : : // At first, the range partition is unbounded, we can insert any data into it. : // We insert 100 row > nit: for multi-line function calls, we generally use four spaces, per the C Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2404 PS7, Line 2404: master_addr, > nit: maybe pull this out into its own variable and reuse it everywhere Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@489 PS7, Line 489: values; > nit: how about, "an empty row will be used"? Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@653 PS7, Line 653: "'EXCLUSIVE_BOUND' can be received"); : } : if (!boost::iequals(FLAGS_upper_bo > nit: We don't need the Substitute() here. Could just be, "wrong type of ran Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@694 PS7, Line 694: per_b > nit: for readability, rather than passing boolean arguments, we generally t Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@837 PS7, Line 837: kTableRangeLowerBoundArg, : "String representation of lower bound of " > Could you also add that if the parameter is an empty array, the partition w Done -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 8 Gerrit-Owner: honeyhexin <honeyhe...@sohu.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <ocla...@gmail.com> Gerrit-Reviewer: honeyhexin <honeyhe...@sohu.com> Gerrit-Comment-Date: Wed, 24 Jul 2019 14:23:02 +0000 Gerrit-HasComments: Yes
