diff --git a/python/kudu/tests/test_client.py b/python/kudu/tests/test_client.py index 89d5b62363..31830514a4 100644 --- a/python/kudu/tests/test_client.py +++ b/python/kudu/tests/test_client.py @@ -349,6 +349,9 @@ def test_alter_table_add_drop_partition(self): # Add Range Partition table = self.client.table(self.ex_table) alterer = self.client.new_table_alterer(table) + # Drop the unbounded range partition. + alterer.drop_range_partition() + # Add a partition from 0 to 100 alterer.add_range_partition( lower_bound={'key': 0}, upper_bound={'key': 100} @@ -357,10 +360,13 @@ def test_alter_table_add_drop_partition(self): # TODO(jtbirdsell): Once C++ client can list partition schema # then this test should confirm that the partition was added. alterer = self.client.new_table_alterer(table) + # Drop the partition from 0 to 100 alterer.drop_range_partition( lower_bound={'key': 0}, upper_bound={'key': 100} ) + # Add back the unbounded range partition + alterer.add_range_partition() table = alterer.alter() diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc index e28f8bf8b5..6ae9be27b3 100644 --- a/src/kudu/integration-tests/alter_table-test.cc +++ b/src/kudu/integration-tests/alter_table-test.cc @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -1503,6 +1504,320 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid) { ASSERT_EQ(100, CountTableRows(table.get())); } +// Attempts to exhaustively check all cases of single-column range partition +// conflicts for ALTER TABLE ADD RANGE PARTITION ops involving two ranges. +// +// Also tests some cases of DROP RANGE PARTITION where possible, but the +// coverage is not exhaustive (the state space for invalid add/drop combinations +// is much bigger than for add/add combinations). +// +// Regression test for KUDU-1792 +TEST_F(AlterTableTest, TestAddRangePartitionConflictExhaustive) { + unique_ptr table_alterer; + + // CREATE TABLE t (c0 INT PRIMARY KEY) + // PARTITION BY + // RANGE (c0) (); + string table_name = "test-alter-range-partitioning-invalid-unbounded"; + unique_ptr table_creator(client_->NewTableCreator()); + ASSERT_OK(table_creator->table_name(table_name) + .schema(&schema_) + .set_range_partition_columns({ "c0" }) + .num_replicas(1) + .Create()); + shared_ptr table; + ASSERT_OK(client_->OpenTable(table_name, &table)); + + // Drop the default UNBOUNDED tablet in order to start with a table with no ranges. + table_alterer.reset(client_->NewTableAlterer(table_name)); + ASSERT_OK(table_alterer->DropRangePartition(schema_.NewRow(), schema_.NewRow()) + ->wait(true)->Alter()); + + // Turns an optional value into a row for the table. + auto fill_row = [&] (boost::optional value) -> unique_ptr { + unique_ptr row(schema_.NewRow()); + if (value) { + CHECK_OK(row->SetInt32("c0", *value)); + } + return row; + }; + + // Attempts to add a range partition to the table with the specified bounds. + auto add_range_partition = [&] (boost::optional lower_bound, + boost::optional upper_bound) -> Status { + table_alterer.reset(client_->NewTableAlterer(table_name)); + return table_alterer->AddRangePartition(fill_row(lower_bound).release(), + fill_row(upper_bound).release()) + ->wait(false) + ->Alter(); + }; + + // Attempts to drop a range partition to the table with the specified bounds. + auto drop_range_partition = [&] (boost::optional lower_bound, + boost::optional upper_bound) -> Status { + table_alterer.reset(client_->NewTableAlterer(table_name)); + return table_alterer->DropRangePartition(fill_row(lower_bound).release(), + fill_row(upper_bound).release()) + ->wait(false) + ->Alter(); + }; + + // Attempts to add two range partitions to the table in a single transaction. + auto add_range_partitions = [&] (boost::optional a_lower_bound, + boost::optional a_upper_bound, + boost::optional b_lower_bound, + boost::optional b_upper_bound) -> Status { + table_alterer.reset(client_->NewTableAlterer(table_name)); + return table_alterer->AddRangePartition(fill_row(a_lower_bound).release(), + fill_row(a_upper_bound).release()) + ->AddRangePartition(fill_row(b_lower_bound).release(), + fill_row(b_upper_bound).release()) + ->wait(false) + ->Alter(); + }; + + // Attempts to add and drop two range partitions in a single transaction. + auto add_drop_range_partitions = [&] (boost::optional a_lower_bound, + boost::optional a_upper_bound, + boost::optional b_lower_bound, + boost::optional b_upper_bound) -> Status { + table_alterer.reset(client_->NewTableAlterer(table_name)); + return table_alterer->AddRangePartition(fill_row(a_lower_bound).release(), + fill_row(a_upper_bound).release()) + ->DropRangePartition(fill_row(b_lower_bound).release(), + fill_row(b_upper_bound).release()) + ->wait(false) + ->Alter(); + }; + + auto bounds_to_string = [] (boost::optional lower_bound, + boost::optional upper_bound) -> string { + if (!lower_bound && !upper_bound) { + return "UNBOUNDED"; + } + if (!lower_bound) { + return strings::Substitute("VALUES < $0", *upper_bound); + } + if (!upper_bound) { + return strings::Substitute("VALUES >= $0", *lower_bound); + } + return strings::Substitute("$0 <= VALUES < $1", *lower_bound, *upper_bound); + }; + + // Checks that b conflicts with a, when added in that order. + auto do_expect_range_partitions_conflict = [&] (boost::optional a_lower_bound, + boost::optional a_upper_bound, + boost::optional b_lower_bound, + boost::optional b_upper_bound) { + SCOPED_TRACE(strings::Substitute("b: $0", bounds_to_string(b_lower_bound, b_upper_bound))); + SCOPED_TRACE(strings::Substitute("a: $0", bounds_to_string(a_lower_bound, a_upper_bound))); + + // Add a then add b. + ASSERT_OK(add_range_partition(a_lower_bound, a_upper_bound)); + Status s = add_range_partition(b_lower_bound, b_upper_bound); + ASSERT_FALSE(s.ok()); + ASSERT_STR_CONTAINS(s.ToString(), + "New range partition conflicts with existing range partition"); + // Clean up by removing a. + ASSERT_OK(drop_range_partition(a_lower_bound, a_upper_bound)); + + // Add a and b in the same transaction. + s = add_range_partitions(a_lower_bound, a_upper_bound, + b_lower_bound, b_upper_bound); + ASSERT_FALSE(s.ok()); + ASSERT_STR_CONTAINS(s.ToString(), + "New range partition conflicts with another new range partition"); + + // To get some extra coverage of DROP RANGE PARTITION, check if the two + // ranges are not equal, and if so, check that adding one and dropping the + // other fails. + + if (a_lower_bound != b_lower_bound || a_upper_bound != b_upper_bound) { + // Add a then drop b. + ASSERT_OK(add_range_partition(a_lower_bound, a_upper_bound)); + Status s = drop_range_partition(b_lower_bound, b_upper_bound); + ASSERT_FALSE(s.ok()); + ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range partition step"); + // Clean up by removing a. + ASSERT_OK(drop_range_partition(a_lower_bound, a_upper_bound)); + + // Add a and drop b in a single transaction. + s = add_drop_range_partitions(a_lower_bound, a_upper_bound, + b_lower_bound, b_upper_bound); + ASSERT_FALSE(s.ok()); + ASSERT_STR_CONTAINS(s.ToString(), + "No range partition found for drop range partition step"); + } + }; + + // Checks that two range partitions conflict. + auto expect_range_partitions_conflict = [&] (boost::optional a_lower_bound, + boost::optional a_upper_bound, + boost::optional b_lower_bound, + boost::optional b_upper_bound) { + do_expect_range_partitions_conflict(a_lower_bound, a_upper_bound, + b_lower_bound, b_upper_bound); + do_expect_range_partitions_conflict(b_lower_bound, b_upper_bound, + a_lower_bound, a_upper_bound); + }; + + /// Bounded / Bounded + + // [----------) + // [----------) + expect_range_partitions_conflict(0, 100, 0, 100); + + // [----------) + // [----------) + expect_range_partitions_conflict(0, 100, 50, 150); + + // [----------) + // [------) + expect_range_partitions_conflict(0, 100, 0, 50); + + // [----------) + // [------) + expect_range_partitions_conflict(0, 100, 50, 100); + + // [----------) + // [------) + expect_range_partitions_conflict(0, 100, 25, 75); + + /// Bounded / Unbounded Above + + // [----------) + // [--------------> + expect_range_partitions_conflict(0, 100, -1, boost::none); + + // [----------) + // [--------------> + expect_range_partitions_conflict(0, 100, 0, boost::none); + + // [----------) + // [-------------> + expect_range_partitions_conflict(0, 100, 1, boost::none); + + // [----------) + // [-------------> + expect_range_partitions_conflict(0, 100, 50, boost::none); + + // [----------) + // [---------> + expect_range_partitions_conflict(0, 100, 99, boost::none); + + /// Bounded / Unbounded Below + + // [----------) + // <-------) + expect_range_partitions_conflict(0, 100, boost::none, 1); + + // [----------) + // <------------) + expect_range_partitions_conflict(0, 100, boost::none, 50); + + // [----------) + // <-----------------) + expect_range_partitions_conflict(0, 100, boost::none, 100); + + // [----------) + // <-------------------) + expect_range_partitions_conflict(0, 100, boost::none, 125); + + /// Bounded / Unbounded + + // [----------) + // <-------------------> + expect_range_partitions_conflict(0, 100, boost::none, boost::none); + + /// Bounded / Single Value + + // [----------) + // | + expect_range_partitions_conflict(0, 100, 0, 1); + + // [----------) + // | + expect_range_partitions_conflict(0, 100, 25, 26); + + // [----------) + // | + expect_range_partitions_conflict(0, 100, 99, 100); + + /// Unbounded Above / Unbounded Above + + // [----------> + // [----------> + expect_range_partitions_conflict(0, boost::none, -10, boost::none); + + // [----------> + // [----------> + expect_range_partitions_conflict(0, boost::none, 0, boost::none); + + /// Unbounded Above / Unbounded Below + + // [----------> + // <----------) + expect_range_partitions_conflict(0, boost::none, boost::none, 100); + + // [----------> + // <-------) + expect_range_partitions_conflict(0, boost::none, boost::none, 1); + + /// Unbounded Above / Unbounded + + // [----------> + // <----------> + expect_range_partitions_conflict(0, boost::none, boost::none, boost::none); + + /// Unbounded Above / Single Value + + // [----------> + // | + expect_range_partitions_conflict(0, boost::none, 0, 1); + + // [----------> + // | + expect_range_partitions_conflict(0, boost::none, 100, 101); + + /// Unbounded Below / Unbounded Below + + // <----------) + // <----------) + expect_range_partitions_conflict(boost::none, 100, boost::none, 100); + + // <----------) + // <-----) + expect_range_partitions_conflict(boost::none, 100, boost::none, 50); + + /// Unbounded Below / Unbounded + + // <----------) + // <----------> + expect_range_partitions_conflict(boost::none, 100, boost::none, boost::none); + + /// Unbounded Below / Single Value + + // <----------) + // | + expect_range_partitions_conflict(boost::none, 100, 50, 51); + + // <----------) + // | + expect_range_partitions_conflict(boost::none, 100, 99, 100); + + /// Unbounded / Unbounded + + // <----------> + // <----------> + expect_range_partitions_conflict(boost::none, boost::none, boost::none, boost::none); + + /// Single Value / Single Value + + // | + // | + expect_range_partitions_conflict(0, 1, 0, 1); +} + TEST_F(ReplicatedAlterTableTest, TestReplicatedAlter) { const int kNumRows = 100; InsertRows(0, kNumRows); diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 3df0d465a2..1e7fb68695 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -1451,7 +1451,8 @@ Status CatalogManager::ApplyAlterPartitioningSteps( auto existing_iter = existing_tablets.upper_bound(lower_bound); if (existing_iter != existing_tablets.end()) { TabletMetadataLock metadata(existing_iter->second, TabletMetadataLock::READ); - if (metadata.data().pb.partition().partition_key_start() < upper_bound) { + if (upper_bound.empty() || + metadata.data().pb.partition().partition_key_start() < upper_bound) { return Status::InvalidArgument( "New range partition conflicts with existing range partition", partition_schema.RangePartitionDebugString(*ops[0].split_row, *ops[1].split_row)); @@ -1459,7 +1460,8 @@ Status CatalogManager::ApplyAlterPartitioningSteps( } if (existing_iter != existing_tablets.begin()) { TabletMetadataLock metadata(std::prev(existing_iter)->second, TabletMetadataLock::READ); - if (metadata.data().pb.partition().partition_key_end() > lower_bound) { + if (metadata.data().pb.partition().partition_key_end().empty() || + metadata.data().pb.partition().partition_key_end() > lower_bound) { return Status::InvalidArgument( "New range partition conflicts with existing range partition", partition_schema.RangePartitionDebugString(*ops[0].split_row, *ops[1].split_row)); @@ -1470,7 +1472,8 @@ Status CatalogManager::ApplyAlterPartitioningSteps( auto new_iter = new_tablets.upper_bound(lower_bound); if (new_iter != new_tablets.end()) { const auto& metadata = new_iter->second->mutable_metadata()->dirty(); - if (metadata.pb.partition().partition_key_start() < upper_bound) { + if (upper_bound.empty() || + metadata.pb.partition().partition_key_start() < upper_bound) { return Status::InvalidArgument( "New range partition conflicts with another new range partition", partition_schema.RangePartitionDebugString(*ops[0].split_row, *ops[1].split_row)); @@ -1478,7 +1481,8 @@ Status CatalogManager::ApplyAlterPartitioningSteps( } if (new_iter != new_tablets.begin()) { const auto& metadata = std::prev(new_iter)->second->mutable_metadata()->dirty(); - if (metadata.pb.partition().partition_key_end() > lower_bound) { + if (metadata.pb.partition().partition_key_end().empty() || + metadata.pb.partition().partition_key_end() > lower_bound) { return Status::InvalidArgument( "New range partition conflicts with another new range partition", partition_schema.RangePartitionDebugString(*ops[0].split_row, *ops[1].split_row));