feat: implement fieldttl for the hash family (#2040)

Fix some corner case bug with set family as well.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

src/core/dense_set.cc

@@ -413,6 +413,8 @@ void DenseSet::AddUnique(void* obj, bool has_ttl, uint64_t hashcode) {
 
 auto DenseSet::Find2(const void* ptr, uint32_t bid, uint32_t cookie)
     -> tuple<size_t, DensePtr*, DensePtr*> {
+  DCHECK_LT(bid, entries_.size());
+
   DensePtr* curr = &entries_[bid];
   ExpireIfNeeded(nullptr, curr);
 

src/core/dense_set.h

@@ -3,6 +3,7 @@
 //
 #pragma once
 
+#include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <functional>
@@ -272,6 +273,9 @@ class DenseSet {
   void* FindInternal(const void* obj, uint64_t hashcode, uint32_t cookie) const;
 
   IteratorBase FindIt(const void* ptr, uint32_t cookie) {
+    if (Empty())
+      return IteratorBase{};
+
     auto [bid, _, curr] = Find2(ptr, BucketId(ptr, cookie), cookie);
     if (curr) {
       return IteratorBase(this, entries_.begin() + bid, curr);
@@ -319,6 +323,7 @@ class DenseSet {
   }
 
   uint32_t BucketId(uint64_t hash) const {
+    assert(capacity_log_ > 0);
     return hash >> (64 - capacity_log_);
   }
 

src/core/string_map.cc

@@ -105,15 +105,6 @@ void StringMap::Clear() {
   ClearInternal();
 }
 
-sds StringMap::Find(std::string_view key) {
-  uint64_t hashcode = Hash(&key, 1);
-  sds str = (sds)FindInternal(&key, hashcode, 1);
-  if (!str)
-    return nullptr;
-
-  return GetValue(str);
-}
-
 std::pair<sds, sds> StringMap::RandomPair() {
   auto it = begin();
   it += rand() % Size();

src/core/string_map.h

@@ -49,7 +49,10 @@ class StringMap : public DenseSet {
     iterator() : IteratorBase() {
     }
 
-    iterator(DenseSet* owner, bool is_end) : IteratorBase(owner, is_end) {
+    explicit iterator(const IteratorBase& o) : IteratorBase(o) {
+    }
+
+    iterator(DenseSet* owner) : IteratorBase(owner, false) {
     }
 
     detail::SdsPair operator->() const {
@@ -88,12 +91,18 @@ class StringMap : public DenseSet {
     }
 
     bool operator==(const iterator& b) const {
-      return curr_list_ == b.curr_list_;
+      if (owner_ == nullptr && b.owner_ == nullptr) {  // to allow comparison with end()
+        return true;
+      }
+      return owner_ == b.owner_ && curr_entry_ == b.curr_entry_;
     }
 
     bool operator!=(const iterator& b) const {
       return !(*this == b);
     }
+
+    using IteratorBase::ExpiryTime;
+    using IteratorBase::HasExpiry;
   };
 
   // Returns true if field was added
@@ -111,16 +120,18 @@ class StringMap : public DenseSet {
   /// @brief  Returns value of the key or nullptr if key not found.
   /// @param key
   /// @return sds
-  sds Find(std::string_view key);
+  iterator Find(std::string_view member) {
+    return iterator{FindIt(&member, 1)};
+  }
 
   void Clear();
 
   iterator begin() {
-    return iterator{this, false};
+    return iterator{this};
   }
 
   iterator end() {
-    return iterator{this, true};
+    return iterator{};
   }
 
   // Returns a random key value pair.

src/core/string_map_test.cc

@@ -72,9 +72,10 @@ class StringMapTest : public ::testing::Test {
 TEST_F(StringMapTest, Basic) {
   EXPECT_TRUE(sm_->AddOrUpdate("foo", "bar"));
   EXPECT_TRUE(sm_->Contains("foo"));
-  EXPECT_STREQ("bar", sm_->Find("foo"));
+  auto it = sm_->Find("foo");
+  EXPECT_STREQ("bar", it->second);
 
-  auto it = sm_->begin();
+  it = sm_->begin();
   EXPECT_STREQ("foo", it->first);
   EXPECT_STREQ("bar", it->second);
   ++it;
@@ -160,7 +161,7 @@ TEST_F(StringMapTest, ReallocIfNeeded) {
 
   EXPECT_EQ(sm_->Size(), 1000);
   for (size_t i = 0; i < 1000; i++)
-    EXPECT_EQ(sm_->Find(build_str(i * 10)), build_str(i * 10 + 1));
+    EXPECT_EQ(sm_->Find(build_str(i * 10))->second, build_str(i * 10 + 1));
 }
 
 }  // namespace dfly

src/server/generic_family.cc

@@ -20,6 +20,7 @@ extern "C" {
 #include "server/container_utils.h"
 #include "server/engine_shard_set.h"
 #include "server/error.h"
+#include "server/hset_family.h"
 #include "server/journal/journal.h"
 #include "server/rdb_extensions.h"
 #include "server/rdb_load.h"
@@ -628,16 +629,17 @@ OpResult<long> OpFieldTtl(Transaction* t, EngineShard* shard, string_view key, s
   if (!IsValid(it))
     return -2;
 
-  if (it->second.ObjType() != OBJ_SET)  // TODO: to finish for hashes.
+  if (it->second.ObjType() != OBJ_SET && it->second.ObjType() != OBJ_HASH)
     return OpStatus::WRONG_TYPE;
 
+  int32_t res = -1;
   if (it->second.ObjType() == OBJ_SET) {
-    int32_t res = SetFamily::FieldExpireTime(db_cntx, it->second, field);
-    return res <= 0 ? res : int32_t(res - MemberTimeSeconds(db_cntx.time_now_ms));
+    res = SetFamily::FieldExpireTime(db_cntx, it->second, field);
+  } else {
+    DCHECK_EQ(OBJ_HASH, it->second.ObjType());
+    res = HSetFamily::FieldExpireTime(db_cntx, it->second, field);
   }
-
-  // TODO: to finish with hash family.
-  return OpStatus::INVALID_VALUE;
+  return res <= 0 ? res : int32_t(res - MemberTimeSeconds(db_cntx.time_now_ms));
 }
 
 }  // namespace

src/server/generic_family_test.cc

@@ -606,10 +606,13 @@ TEST_F(GenericFamilyTest, FieldTtl) {
   TEST_current_time_ms = kMemberExpiryBase * 1000;  // to reset to test time.
   EXPECT_THAT(Run({"saddex", "key", "1", "val1"}), IntArg(1));
   EXPECT_THAT(Run({"saddex", "key", "2", "val2"}), IntArg(1));
+  EXPECT_THAT(Run({"sadd", "key", "val3"}), IntArg(1));
+
   EXPECT_EQ(-2, CheckedInt({"fieldttl", "nokey", "val1"}));  // key not found
   EXPECT_EQ(-3, CheckedInt({"fieldttl", "key", "bar"}));     // field not found
   EXPECT_EQ(1, CheckedInt({"fieldttl", "key", "val1"}));
   EXPECT_EQ(2, CheckedInt({"fieldttl", "key", "val2"}));
+  EXPECT_EQ(-1, CheckedInt({"fieldttl", "key", "val3"}));
 
   AdvanceTime(1100);
   EXPECT_EQ(-3, CheckedInt({"fieldttl", "key", "val1"}));
@@ -617,6 +620,13 @@ TEST_F(GenericFamilyTest, FieldTtl) {
 
   Run({"set", "str", "val"});
   EXPECT_THAT(Run({"fieldttl", "str", "bar"}), ErrArg("wrong"));
+
+  EXPECT_EQ(2, CheckedInt({"HSETEX", "k2", "1", "f1", "v1", "f2", "v2"}));
+  EXPECT_EQ(1, CheckedInt({"HSET", "k2", "f3", "v3"}));
+
+  EXPECT_EQ(1, CheckedInt({"fieldttl", "k2", "f1"}));
+  EXPECT_EQ(-1, CheckedInt({"fieldttl", "k2", "f3"}));
+  EXPECT_EQ(-3, CheckedInt({"fieldttl", "k2", "f4"}));
 }
 
 }  // namespace dfly

src/server/hset_family.cc

@@ -233,7 +233,10 @@ OpStatus OpIncrBy(const OpArgs& op_args, string_view key, string_view field, Inc
 
     sds val = nullptr;
     if (!inserted) {
-      val = sm->Find(field);
+      auto it = sm->Find(field);
+      if (it != sm->end()) {
+        val = it->second;
+      }
     }
 
     optional<string_view> sv;
@@ -451,9 +454,9 @@ OpResult<vector<OptStr>> OpMGet(const OpArgs& op_args, std::string_view key, Cmd
     StringMap* sm = GetStringMap(pv, op_args.db_cntx);
 
     for (size_t i = 0; i < fields.size(); ++i) {
-      sds val = sm->Find(ToSV(fields[i]));
-      if (val) {
-        result[i].emplace(val, sdslen(val));
+      auto it = sm->Find(ToSV(fields[i]));
+      if (it != sm->end()) {
+        result[i].emplace(it->second, sdslen(it->second));
       }
     }
   }
@@ -495,7 +498,7 @@ OpResult<int> OpExist(const OpArgs& op_args, string_view key, string_view field)
   DCHECK_EQ(kEncodingStrMap2, pv.Encoding());
   StringMap* sm = GetStringMap(pv, op_args.db_cntx);
 
-  return sm->Find(field) ? 1 : 0;
+  return sm->Contains(field) ? 1 : 0;
 };
 
 OpResult<string> OpGet(const OpArgs& op_args, string_view key, string_view field) {
@@ -518,12 +521,12 @@ OpResult<string> OpGet(const OpArgs& op_args, string_view key, string_view field
 
   DCHECK_EQ(pv.Encoding(), kEncodingStrMap2);
   StringMap* sm = GetStringMap(pv, op_args.db_cntx);
-  sds val = sm->Find(field);
+  auto it = sm->Find(field);
 
-  if (!val)
+  if (it == sm->end())
     return OpStatus::KEY_NOTFOUND;
 
-  return string(val, sdslen(val));
+  return string(it->second, sdslen(it->second));
 }
 
 OpResult<vector<string>> OpGetAll(const OpArgs& op_args, string_view key, uint8_t mask) {
@@ -599,8 +602,8 @@ OpResult<size_t> OpStrLen(const OpArgs& op_args, string_view key, string_view fi
   DCHECK_EQ(pv.Encoding(), kEncodingStrMap2);
   StringMap* sm = GetStringMap(pv, op_args.db_cntx);
 
-  sds res = sm->Find(field);
-  return res ? sdslen(res) : 0;
+  auto it = sm->Find(field);
+  return it != sm->end() ? sdslen(it->second) : 0;
 }
 
 struct OpSetParams {
@@ -1201,4 +1204,25 @@ StringMap* HSetFamily::ConvertToStrMap(uint8_t* lp) {
 
   return sm;
 }
+
+// returns -1 if no expiry is associated with the field, -3 if no field is found.
+int32_t HSetFamily::FieldExpireTime(const DbContext& db_context, const PrimeValue& pv,
+                                    std::string_view field) {
+  DCHECK_EQ(OBJ_HASH, pv.ObjType());
+
+  if (pv.Encoding() == kEncodingListPack) {
+    uint8_t intbuf[LP_INTBUF_SIZE];
+    uint8_t* lp = (uint8_t*)pv.RObjPtr();
+    optional<string_view> res = LpFind(lp, field, intbuf);
+    return res ? -1 : -3;
+  } else {
+    StringMap* string_map = (StringMap*)pv.RObjPtr();
+    string_map->set_time(MemberTimeSeconds(db_context.time_now_ms));
+    auto it = string_map->Find(field);
+    if (it == string_map->end())
+      return -3;
+    return it.HasExpiry() ? it.ExpiryTime() : -1;
+  }
+}
+
 }  // namespace dfly

src/server/hset_family.h

@@ -8,6 +8,7 @@
 
 #include "facade/op_status.h"
 #include "server/common.h"
+#include "server/table.h"
 
 namespace dfly {
 
@@ -25,6 +26,9 @@ class HSetFamily {
   // Does not free lp.
   static StringMap* ConvertToStrMap(uint8_t* lp);
 
+  static int32_t FieldExpireTime(const DbContext& db_context, const PrimeValue& pv,
+                                 std::string_view field);
+
  private:
   // TODO: to move it to anonymous namespace in cc file.
 

src/server/search/doc_accessors.cc

@@ -87,7 +87,8 @@ SearchDocData ListPackAccessor::Serialize(const search::Schema& schema) const {
 }
 
 string_view StringMapAccessor::GetString(string_view active_field) const {
-  return SdsToSafeSv(hset_->Find(active_field));
+  auto it = hset_->Find(active_field);
+  return it != hset_->end() ? it->second : ""sv;
 }
 
 BaseAccessor::VectorInfo StringMapAccessor::GetVector(string_view active_field) const {

src/server/set_family.cc

@@ -323,7 +323,7 @@ int32_t GetExpiry(const DbContext& db_context, const SetType& st, string_view me
     if (it == ss->end())
       return -3;
 
-    return it.ExpiryTime();
+    return it.HasExpiry() ? it.ExpiryTime() : -1;
   } else {
     // Old encoding, does not support expiry.
     return -1;