This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push: new e0f96b9c8 KUDU-2671 forward-looking provision for AddRangePartition e0f96b9c8 is described below commit e0f96b9c838e33b93690a76f771d1eeaf3a99222 Author: Alexey Serbin <ale...@apache.org> AuthorDate: Thu Jul 7 17:03:55 2022 -0700 KUDU-2671 forward-looking provision for AddRangePartition The way how the information on range-specific hash schema is specified in AlterTableRequestPB::AddRangePartition introduced by [1] assumes there should not be an empty custom hash schema for a range when the table-wide hash schema isn't empty. As of now, the assumption holds true since there is an artificial restriction on the variability of the number of dimensions in per-range hash schemas in a table introduced by changelist [2]. However, once the restriction introduced in [2] is removed, the current type of the AddRangePartition::custom_hash_schema deprives the code to tell between the case of a range-specific hash schema with zero hash dimensions (a.k.a. empty hash schema, i.e. no hash bucketing at all) and the case of table-wide hash schema for a newly added range partition. This patch fixes the deficiency, so now it's possible to call has_custom_hash_schema() and hasCustomHashSchema() on AddRangePartition object in C++ and Java code correspondingly, not relying on the emptiness of the repeated field representing the set of hash dimensions for the range-specific hash schema. This patch would break backwards compatibility if there were a version of Kudu released with the change introduced in changlist [1], but that's not the case. So, it was possible to simply change the type of the AddRangePartition::custom_hash_schema field. [1] https://github.com/apache/kudu/commit/11db3f28b36d92ce1515bcaace51a3586838abcb [2] https://github.com/apache/kudu/commit/6998193e69eeda497f912d1d806470c95b591ad4 Change-Id: I30f654443c7f51a76dea9d980588b399b06c2dd1 Reviewed-on: http://gerrit.cloudera.org:8080/18713 Tested-by: Alexey Serbin <ale...@apache.org> Reviewed-by: Mahesh Reddy <mre...@cloudera.com> Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com> Reviewed-by: Alexey Serbin <ale...@apache.org> --- .../java/org/apache/kudu/client/AlterTableOptions.java | 8 ++------ src/kudu/client/table_alterer-internal.cc | 2 +- src/kudu/master/catalog_manager.cc | 7 ++++--- src/kudu/master/master-test.cc | 8 +++++--- src/kudu/master/master.proto | 14 ++++++++++++-- 5 files changed, 24 insertions(+), 15 deletions(-) diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java b/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java index 1603d0801..e5e8f193f 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java @@ -385,12 +385,8 @@ public class AlterTableOptions { new Operation.OperationsEncoder().encodeLowerAndUpperBounds( range.getLowerBound(), range.getUpperBound(), range.getLowerBoundType(), range.getUpperBoundType())); - for (org.apache.kudu.Common.PartitionSchemaPB.HashBucketSchemaPB hashSchema : - range.toPB().getHashSchemaList()) { - Common.PartitionSchemaPB.HashBucketSchemaPB.Builder hbs = - rangeBuilder.addCustomHashSchemaBuilder(); - hbs.mergeFrom(hashSchema); - } + rangeBuilder.getCustomHashSchemaBuilder().addAllHashSchema( + range.toPB().getHashSchemaList()); step.setAddRangePartition(rangeBuilder); if (!pb.hasSchema()) { pb.setSchema(ProtobufHelper.schemaToPb(range.getLowerBound().getSchema(), diff --git a/src/kudu/client/table_alterer-internal.cc b/src/kudu/client/table_alterer-internal.cc index 8e43ebfe2..68a8b8def 100644 --- a/src/kudu/client/table_alterer-internal.cc +++ b/src/kudu/client/table_alterer-internal.cc @@ -183,7 +183,7 @@ Status KuduTableAlterer::Data::ToRequest(AlterTableRequestPB* req) { for (const auto& hash_dimension : partition_data->hash_schema_) { auto* custom_hash_schema_pb = pb_step->mutable_add_range_partition()-> - add_custom_hash_schema(); + mutable_custom_hash_schema()->add_hash_schema(); for (const auto& column_name : hash_dimension.column_names) { custom_hash_schema_pb->add_columns()->set_name(column_name); } diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 17411b1c3..534405aa6 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -2678,12 +2678,13 @@ Status CatalogManager::ApplyAlterPartitioningSteps( const pair<KuduPartialRow, KuduPartialRow> range_bound = { *ops[0].split_row, *ops[1].split_row }; if (step.type() == AlterTableRequestPB::ADD_RANGE_PARTITION) { - const auto& custom_hash_schema_pb = - step.add_range_partition().custom_hash_schema(); - if (!FLAGS_enable_per_range_hash_schemas || custom_hash_schema_pb.empty()) { + if (!FLAGS_enable_per_range_hash_schemas || + !step.add_range_partition().has_custom_hash_schema()) { RETURN_NOT_OK(partition_schema.CreatePartitions( {}, { range_bound }, schema, &partitions)); } else { + const auto& custom_hash_schema_pb = + step.add_range_partition().custom_hash_schema().hash_schema(); const Schema schema = client_schema.CopyWithColumnIds(); PartitionSchema::HashSchema hash_schema; RETURN_NOT_OK(PartitionSchema::ExtractHashSchemaFromPB( diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc index f66ddfc9b..7105b401d 100644 --- a/src/kudu/master/master-test.cc +++ b/src/kudu/master/master-test.cc @@ -976,7 +976,9 @@ TEST_P(AlterTableWithRangeSpecificHashSchema, TestAlterTableWithDifferentHashDim splits_encoder.Add(RowOperationsPB::RANGE_LOWER_BOUND, lower); splits_encoder.Add(RowOperationsPB::RANGE_UPPER_BOUND, upper); for (const auto& hash_dimension: custom_range_hash_schema) { - auto* hash_dimension_pb = step->mutable_add_range_partition()->add_custom_hash_schema(); + auto* hash_dimension_pb = + step->mutable_add_range_partition()->mutable_custom_hash_schema()-> + add_hash_schema(); for (const string& col_name: hash_dimension.columns) { hash_dimension_pb->add_columns()->set_name(col_name); } @@ -1078,8 +1080,8 @@ TEST_F(MasterTest, AlterTableAddAndDropRangeWithSpecificHashSchema) { enc.Add(RowOperationsPB::RANGE_LOWER_BOUND, lower); enc.Add(RowOperationsPB::RANGE_UPPER_BOUND, upper); for (const auto& hash_dimension: custom_hash_schema) { - auto* hash_dimension_pb = - step->mutable_add_range_partition()->add_custom_hash_schema(); + auto* hash_dimension_pb = step->mutable_add_range_partition()-> + mutable_custom_hash_schema()->add_hash_schema(); for (const auto& col_name: hash_dimension.columns) { hash_dimension_pb->add_columns()->set_name(col_name); } diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index f3f65657a..e660a74df 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -725,6 +725,15 @@ message AlterTableRequestPB { optional ColumnSchemaDeltaPB delta = 1; } message AddRangePartition { + // A structure to define range-specific hash schema. This separate type + // exists to distinguish from an empty hash schema (i.e. no hash bucketing) + // and the absence of range-specific hash schema when a range partition + // uses the table-wide hash schema instead. Otherwise, using a field of + // repeated HashBucketSchemaPB wouldn't allow to tell between those cases. + message CustomHashSchema { + repeated PartitionSchemaPB.HashBucketSchemaPB hash_schema = 1; + } + // A set of row operations containing the lower and upper range bound for // the range partition to add or drop. optional RowOperationsPB range_bounds = 1; @@ -733,8 +742,9 @@ message AlterTableRequestPB { // of the tablet's replicas. optional string dimension_label = 2; - // The custom hash partition schema for the range if specified. - repeated PartitionSchemaPB.HashBucketSchemaPB custom_hash_schema = 3; + // The custom hash partition schema for the range, if specified. If absent, + // the range uses table-wide hash schema. + optional CustomHashSchema custom_hash_schema = 3; } message DropRangePartition { // A set of row operations containing the lower and upper range bound for