Adar Dembo 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 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@98 PS12, Line 98: lower upper http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@484 PS12, Line 484: Status ConvertToKuduPartialRow(const client::sp::shared_ptr<KuduTable>& table, Could some of TableScanner be reused for this? There's logic there to deserialize JSON into predicates, which seems quite similar. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@592 PS12, Line 592: case KuduColumnSchema::BOOL: { : bool value; : RETURN_NOT_OK_PREPEND( : reader.ExtractBool(values[i], /*field=*/nullptr, &value), : Substitute(kErrorMsgArg, : values[i], : col_name, : KuduColumnSchema::DataTypeToString(type))); : range_bound_partial_row->SetBool(col_name, value); : break; : } > Per https://kudu.apache.org/docs/schema_design.html#primary-keys, boolean Agreed with Andrew, though in this case I wouldn't use FALLTHROUGH_INTENDED, because we generally only use it when the missing 'break' looks like a mistake. For example: switch (foo) { case A: DoA(); break; case B: DoB(); case C: default: DoDefault(); break; When I read this, I notice that case B does something but doesn't have a 'break', which looks like an error. I also notice that case C chains directly into the default case, which seems normal. If the intent for case B was to call DoB() and DoDefault(), FALLTHROUGH_INTENDED is appropriate: switch (foo) { case A: DoA(); break; case B: DoB(); FALLTHROUGH_INTENDED; case C: default: DoDefault(); break; Under the hood, FALLTHROUGH_INTENDED expands to the [[clang::fallthrough]] attribute, which affects compilation with -Wimplicit-fallthrough. Of note: when compiling a "direct chain" with -Wimplicit-fallthrough but without [[clang::fallthrough]], no warning is generated. This further emphasizes that FALLTHROUGH_INTENDED is only needed for non-obvious chaining. See https://clang.llvm.org/docs/AttributeReference.html#fallthrough for details. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@649 PS12, Line 649: if (!boost::iequals(FLAGS_lower_bound_type, "INCLUSIVE_BOUND") && : !boost::iequals(FLAGS_lower_bound_type, "EXCLUSIVE_BOUND")) { : return Status::InvalidArgument( : "wrong type of range lower bound : only 'INCLUSIVE_BOUND' or " : "'EXCLUSIVE_BOUND' can be received"); : } : if (!boost::iequals(FLAGS_upper_bound_type, "INCLUSIVE_BOUND") && : !boost::iequals(FLAGS_upper_bound_type, "EXCLUSIVE_BOUND")) { : return Status::InvalidArgument( : "wrong type of range upper bound : only 'INCLUSIVE_BOUND' or " : "'EXCLUSIVE_BOUND' can be received"); : } Can use a lambda here to deduplicate. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@675 PS12, Line 675: boost::iequals(FLAGS_lower_bound_type, "INCLUSIVE_BOUND") ? : KuduTableCreator::INCLUSIVE_BOUND : : KuduTableCreator::EXCLUSIVE_BOUND; : KuduTableCreator::RangePartitionBound upper_bound_type = : boost::iequals(FLAGS_upper_bound_type, "EXCLUSIVE_BOUND") ? : KuduTableCreator::EXCLUSIVE_BOUND : : KuduTableCreator::INCLUSIVE_BOUND; > nit: per the style guide, indent at 4 spaces for multi-line statements. I'd combine this in with the validation on L649-660. You can imagine a lambda (or helper function) that returns a Status and writes a KuduTableCreator::RangePartitionBound OUT parameter. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@690 PS12, Line 690: Nit: DCHECK_EQ(PartitionAction::DROP, action); http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@843 PS12, Line 843: lower upper http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@846 PS12, Line 846: lower upper -- 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: 12 Gerrit-Owner: honeyhexin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: honeyhexin <[email protected]> Gerrit-Comment-Date: Thu, 25 Jul 2019 19:11:17 +0000 Gerrit-HasComments: Yes
