Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(search): Rax TreeMap #3909

Merged
merged 3 commits into from
Oct 13, 2024
Merged

chore(search): Rax TreeMap #3909

merged 3 commits into from
Oct 13, 2024

Conversation

dranikpg
Copy link
Contributor

Self contained RaxTreeMap that hides rax functions + allocations

Comment on lines 112 to 126
template <typename... Args>
std::pair<FindIterator, bool> try_emplace(std::string_view key, Args&&... args) {
if (auto it = find(key); it)
return {it, false};

void* ptr = mr_->allocate(sizeof(V), alignof(V));
V* data = new (ptr) V(std::forward<Args>(args)...);

V* old = nullptr;
raxInsert(tree_, to_key_ptr(key), key.size(), data, reinterpret_cast<void**>(&old));
assert(old == nullptr);

auto it = std::make_optional(std::make_pair(key, data));
return std::make_pair(FindIterator{it}, true);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so ugly 😭

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Contributor Author

I will replace the built-in absl::flat_hash_map with this data structure and we'll have prefix searches automatically, for suffix/infix searches we'll build a suffix tree on top of this struct

@dranikpg dranikpg marked this pull request as ready for review October 12, 2024 14:42
@dranikpg dranikpg requested a review from romange October 12, 2024 14:42
@dranikpg
Copy link
Contributor Author

Forgot to mention, properly using polymorphic allocators is a pain because we use redis/zmalloc internally 😕 Not sure what to do about it 🤷🏻‍♂️

src/core/search/rax_tree.h Outdated Show resolved Hide resolved
@dranikpg
Copy link
Contributor Author

Ah, I forgot our PMR_NS namespace 😅 Will fix it

std::optional<raxIterator> it;
};

struct FindIterator : public std::optional<std::pair<std::string_view, V&>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment that we use optional here to get "->" operators for free.

}
}

TEST_F(RaxTreeTest, LowerBound) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check how it works with empty string_views, strings. Sometimes redis code has problems with zero length slices that have nullptr pointers

Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

// Test lower bound empty string
vector<string> keys2;
for (auto it = map.lower_bound(""); it != map.end(); ++it)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not enough.
"" actually is non-null string_view. I am talking about string_view{}.
Also the same with find and insert (empty string)

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg merged commit 92217b6 into dragonflydb:main Oct 13, 2024
9 checks passed
@dranikpg dranikpg deleted the rax-tree-map branch October 20, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants