jihuayu commented on code in PR #3567:
URL: https://github.com/apache/kvrocks/pull/3567#discussion_r3794296771
##########
tests/cppunit/types/cuckoo_filter_test.cc:
##########
@@ -743,3 +783,164 @@ TEST_F(RedisCuckooFilterTest,
ExpansionWritesNewFilterIndexPage) {
ASSERT_TRUE(s.ok()) << s.ToString();
EXPECT_EQ(page.size(), expected_page_size);
}
+
+TEST_F(RedisCuckooFilterTest, DeleteMissingKeyReturnsNotFound) {
+ bool deleted = true;
+ auto s = cuckoo_->Delete(*ctx_, key_, "missing", &deleted);
+ EXPECT_TRUE(s.IsNotFound()) << s.ToString();
+ EXPECT_NE(s.ToString().find("Not found"), std::string::npos);
+}
+
+TEST_F(RedisCuckooFilterTest, DeleteBasicClearsOneItem) {
+ reserveAndVerify(key_, 1000, 4, 500, 2);
+ addAndVerify(key_, "item", 1000, 4, 500, 2, 1);
+
+ bool deleted = false;
+ auto s = cuckoo_->Delete(*ctx_, key_, "item", &deleted);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ EXPECT_TRUE(deleted);
+ verifyMetadata(key_, 1000, 4, 500, 2, 0, 1, 1);
+
+ deleted = true;
+ s = cuckoo_->Delete(*ctx_, key_, "item", &deleted);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ EXPECT_FALSE(deleted);
+ verifyMetadata(key_, 1000, 4, 500, 2, 0, 1, 1);
+}
+
+TEST_F(RedisCuckooFilterTest, DeleteDuplicateItemsOneAtATime) {
+ reserveAndVerify(key_, 1000, 4, 500, 2);
+ for (int i = 0; i < 3; ++i) {
+ addAndVerify(key_, "duplicate", 1000, 4, 500, 2, i + 1);
+ }
+
+ for (int i = 0; i < 3; ++i) {
+ bool deleted = false;
+ auto s = cuckoo_->Delete(*ctx_, key_, "duplicate", &deleted);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ EXPECT_TRUE(deleted);
+ verifyMetadata(key_, 1000, 4, 500, 2, 2 - i, 1, i + 1);
+ }
+
+ bool deleted = true;
+ auto s = cuckoo_->Delete(*ctx_, key_, "duplicate", &deleted);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ EXPECT_FALSE(deleted);
+ verifyMetadata(key_, 1000, 4, 500, 2, 0, 1, 3);
+}
Review Comment:
I think these case Go tests is OK, so there’s no need to add C++ test cases.
##########
src/types/redis_cuckoo_chain.h:
##########
@@ -62,6 +65,8 @@ class CuckooChain : public Database {
bool *inserted);
rocksdb::Status commitSubFilterAndMetadata(engine::Context &ctx, const Slice
&user_key, const std::string &ns_key,
CuckooChainMetadata *metadata,
CuckooSubFilter *sub_filter);
+ rocksdb::Status commitDelete(engine::Context &ctx, const Slice &user_key,
const std::string &ns_key,
Review Comment:
I feel that `commitDelete` is a bit odd from an architectural perspective,
as it duplicates some of the logic in `commitSubFilterAndMetadata`. Could we
reuse `commitSubFilterAndMetadata` for the commit-related logic instead?
##########
tests/gocase/unit/type/bloom/cuckoo_filter_test.go:
##########
@@ -107,6 +107,42 @@ func TestCuckooFilter(t *testing.T) {
require.Error(t, rdb.Do(ctx, "cf.add", "key", "item1",
"item2").Err())
})
+ t.Run("Del wrong number of arguments", func(t *testing.T) {
+ require.Error(t, rdb.Do(ctx, "cf.del").Err())
+ require.Error(t, rdb.Do(ctx, "cf.del", "key_only").Err())
+ require.Error(t, rdb.Do(ctx, "cf.del", "key", "item1",
"item2").Err())
+ })
+
+ t.Run("Del missing key", func(t *testing.T) {
+ key := "test_cuckoo_filter_del_missing"
+ require.NoError(t, rdb.Del(ctx, key).Err())
+ require.ErrorContains(t, rdb.Do(ctx, "cf.del", key,
"item").Err(), "Not found")
+ })
+
+ t.Run("Del wrong type", func(t *testing.T) {
+ key := "test_cuckoo_filter_del_wrong_type"
+ require.NoError(t, rdb.Set(ctx, key, "value", 0).Err())
+ require.ErrorContains(t, rdb.Do(ctx, "cf.del", key,
"item").Err(), "WRONGTYPE")
+ })
+
+ t.Run("Del basic", func(t *testing.T) {
Review Comment:
The current tests don't seem to cover cases that span multiple sub-filters.
Could you add a test case for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
