Copilot commented on code in PR #3567:
URL: https://github.com/apache/kvrocks/pull/3567#discussion_r3799944533
##########
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);
+}
Review Comment:
Missing keys should not produce a NotFound error for CF.DEL if we want
RedisBloom-compatible semantics (return 0 and deleted=false). This test
currently expects NotFound and should be updated accordingly.
##########
src/types/redis_cuckoo_chain.cc:
##########
@@ -179,6 +179,45 @@ rocksdb::Status CuckooChain::Add(engine::Context &ctx,
const Slice &user_key, co
return rocksdb::Status::Aborted("filter is full");
}
+rocksdb::Status CuckooChain::Delete(engine::Context &ctx, const Slice
&user_key, const Slice &item, bool *deleted) {
+ *deleted = false;
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ CuckooChainMetadata metadata(false);
+ auto s = getCuckooChainMetadata(ctx, ns_key, &metadata);
+ if (s.IsNotFound()) return rocksdb::Status::NotFound("Not found");
+ if (!s.ok()) return s;
Review Comment:
CF.DEL should be RedisBloom-compatible: when the key does not exist,
RedisBloom returns integer 0 (no error). Returning rocksdb::Status::NotFound
here causes the command layer to emit an error instead of 0, and also diverges
from how BloomChain::MExists handles missing keys (returns OK + false).
This issue also appears on line 197 of the same file.
##########
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")
+ })
Review Comment:
RedisBloom CF.DEL returns integer 0 when the key does not exist; it should
not return a "Not found" error. This test currently asserts the opposite and
will lock in incompatible behavior.
--
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]
