KUDU-1899. Fix support for empty string keys We've never prevented users from inserting a row with "" as a key. However, a faulty assertion in diskrowset.cc caused the tablet server to crash on flush in this case.
This adds a new end-to-end test via the client which inserts, updates, and deletes a row with a "" key. It caused a crash prior to this patch. Change-Id: I779f25afe6d39d91067b1e7c1238797ec2ac0295 Reviewed-on: http://gerrit.cloudera.org:8080/6163 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c11a315e Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c11a315e Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c11a315e Branch: refs/heads/master Commit: c11a315e73a20211caf71f2b3797e71af8449af4 Parents: 8f54f60 Author: Todd Lipcon <[email protected]> Authored: Sun Feb 26 23:39:20 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Tue Feb 28 20:52:51 2017 +0000 ---------------------------------------------------------------------- src/kudu/client/client-test.cc | 80 +++++++++++++++++++++++++++++++++++++ src/kudu/tablet/diskrowset.cc | 3 +- 2 files changed, 81 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c11a315e/src/kudu/client/client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index bc57d6d..f7c8e0a 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -3950,6 +3950,86 @@ TEST_F(ClientTest, TestInsertTooLongEncodedPrimaryKey) { errors[0]->status().ToString()); } +// Test trying to insert a row with an empty string PK. +// Regression test for KUDU-1899. +TEST_F(ClientTest, TestInsertEmptyPK) { + const string kLongValue(10000, 'x'); + const char* kTableName = "empty-pk"; + + // Create and open a table with a three-column composite PK. + unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator()); + KuduSchema schema; + KuduSchemaBuilder schema_builder; + schema_builder.AddColumn("k1")->Type(KuduColumnSchema::STRING)->NotNull(); + schema_builder.AddColumn("v1")->Type(KuduColumnSchema::STRING)->NotNull(); + schema_builder.SetPrimaryKey({"k1"}); + ASSERT_OK(schema_builder.Build(&schema)); + ASSERT_OK(table_creator->table_name(kTableName) + .schema(&schema) + .num_replicas(1) + .set_range_partition_columns({ "k1" }) + .Create()); + shared_ptr<KuduTable> table; + ASSERT_OK(client_->OpenTable(kTableName, &table)); + + // Find the tablet. + scoped_refptr<TabletPeer> tablet_peer; + ASSERT_TRUE(cluster_->mini_tablet_server(0)->server()->tablet_manager()->LookupTablet( + GetFirstTabletId(table.get()), &tablet_peer)); + + // Utility function to get the current value of the row. + const auto ReadRowAsString = [&]() { + vector<string> rows; + ScanTableToStrings(table.get(), &rows); + if (rows.empty()) return string("<none>"); + CHECK_EQ(1, rows.size()); + return rows[0]; + }; + + shared_ptr<KuduSession> session(client_->NewSession()); + ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); + + // Insert a row with empty primary key. + { + unique_ptr<KuduInsert> insert(table->NewInsert()); + ASSERT_OK(insert->mutable_row()->SetString(0, "")); + ASSERT_OK(insert->mutable_row()->SetString(1, "initial")); + ASSERT_OK(session->Apply(insert.release())); + ASSERT_OK(session->Flush()); + } + ASSERT_EQ("(string k1=\"\", string v1=\"initial\")", ReadRowAsString()); + + // Make sure that Flush works properly, and the data is still readable. + ASSERT_OK(tablet_peer->tablet()->Flush()); + ASSERT_EQ("(string k1=\"\", string v1=\"initial\")", ReadRowAsString()); + + // Perform an update. + { + unique_ptr<KuduUpdate> update(table->NewUpdate()); + ASSERT_OK(update->mutable_row()->SetString(0, "")); + ASSERT_OK(update->mutable_row()->SetString(1, "updated")); + ASSERT_OK(session->Apply(update.release())); + ASSERT_OK(session->Flush()); + } + ASSERT_EQ("(string k1=\"\", string v1=\"updated\")", ReadRowAsString()); + ASSERT_OK(tablet_peer->tablet()->FlushAllDMSForTests()); + ASSERT_EQ("(string k1=\"\", string v1=\"updated\")", ReadRowAsString()); + ASSERT_OK(tablet_peer->tablet()->Compact(tablet::Tablet::FORCE_COMPACT_ALL)); + ASSERT_EQ("(string k1=\"\", string v1=\"updated\")", ReadRowAsString()); + + // Perform a delete. + { + unique_ptr<KuduDelete> del(table->NewDelete()); + ASSERT_OK(del->mutable_row()->SetString(0, "")); + ASSERT_OK(session->Apply(del.release())); + ASSERT_OK(session->Flush()); + } + ASSERT_EQ("<none>", ReadRowAsString()); + ASSERT_OK(tablet_peer->tablet()->FlushAllDMSForTests()); + ASSERT_EQ("<none>", ReadRowAsString()); + ASSERT_OK(tablet_peer->tablet()->Compact(tablet::Tablet::FORCE_COMPACT_ALL)); + ASSERT_EQ("<none>", ReadRowAsString()); +} // Check the behavior of the latest observed timestamp when performing // write and read operations. http://git-wip-us.apache.org/repos/asf/kudu/blob/c11a315e/src/kudu/tablet/diskrowset.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/diskrowset.cc b/src/kudu/tablet/diskrowset.cc index 06b3897..014ecee 100644 --- a/src/kudu/tablet/diskrowset.cc +++ b/src/kudu/tablet/diskrowset.cc @@ -186,7 +186,7 @@ Status DiskRowSetWriter::AppendBlock(const RowBlock &block) { } #ifndef NDEBUG - CHECK_LT(Slice(prev_key).compare(enc_key), 0) + CHECK(prev_key.size() == 0 || Slice(prev_key).compare(enc_key) < 0) << KUDU_REDACT(enc_key.ToDebugString()) << " appended to file not > previous key " << KUDU_REDACT(Slice(prev_key).ToDebugString()); #endif @@ -214,7 +214,6 @@ Status DiskRowSetWriter::FinishAndReleaseBlocks(ScopedWritableBlockCloser* close } // Save the last encoded (max) key - CHECK_GT(last_encoded_key_.size(), 0); Slice last_enc_slice(last_encoded_key_); std::string first_encoded_key = key_index_writer()->GetMetaValueOrDie(DiskRowSet::kMinKeyMetaEntryName);
