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

Basic search #1187

Merged
merged 4 commits into from May 9, 2023
Merged

Basic search #1187

merged 4 commits into from May 9, 2023

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented May 6, 2023

Implements brute force search

Basic redis search interface:

  • FT.CREATE with a single index
  • FT.SEARCH

src/server/container_utils.h Outdated Show resolved Hide resolved
src/server/search_family.cc Show resolved Hide resolved

vector<vector<SerializedDocument>> docs(shard_set->size());
cntx->transaction->ScheduleSingleHop([&](Transaction* t, EngineShard* shard) {
OpSearch(t->GetOpArgs(shard), &docs[shard->shard_id()], filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can have some contention here when pushing to the vectors... We have this pattern also in other places in the code as well, but this one is a bit worse because you're pushing to the vector in a loop during the search :)

Copy link
Contributor Author

@dranikpg dranikpg May 7, 2023

Choose a reason for hiding this comment

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

Do you mean that we push in a loop without a resize ahead? We don't have any estimation for the number of docs 🤷🏻 I don't think that's as issue for now (MVP is brute force search, no performance goals)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, sorry that I've been unclear. We're pushing to docs[0] and docs[1] from different threads, but since they're allocated continuously inside docs they're probably on the same cache line. And since each push_back changes the vector's size member, you're going to get contention between the threads.

I agree it's not the end of the world, just wanted to point it out :)

Copy link
Contributor Author

@dranikpg dranikpg May 7, 2023

Choose a reason for hiding this comment

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

Ah, I see, you mean cache padding. Agree, we need a general solution for this for all other places as well:

  • a CachePadded<T> wrapper
  • and possibly use an inlined vector for small thread counts

Transaction currently does all of this with its shard data, but its not re-usable.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg changed the title [WIP WIP WIP] Basic search Basic search May 7, 2023
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
src/server/container_utils.cc Show resolved Hide resolved
src/server/container_utils.h Outdated Show resolved Hide resolved
src/server/container_utils.h Show resolved Hide resolved
class ConnectionContext;

class SearchFamily {
static void FtCreate(CmdArgList args, ConnectionContext* cntx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually there's no need for static-private class members, we can just declare them in an anonymous namespace in the .cc file. Looks like we've already done this in bitops_family:

class BitOpsFamily {
public:
/// @brief Register the function that would be called to operate on user commands.
/// @param registry The location to which the handling functions would be registered.
///
/// We are assuming that this would have a valid registry to work on (i.e this do not point to
/// null!).
static void Register(CommandRegistry* registry);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do it very inconsistently, they're present mostly on the older files. I like when you have a quick overview over the command handlers from the header. Anyways I can remove them

Copy link
Collaborator

Choose a reason for hiding this comment

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

100% your call, just a suggestion. As always, feel free to overrule :)

src/server/search_family.cc Show resolved Hide resolved
src/server/search_family.cc Outdated Show resolved Hide resolved
src/server/search_family_test.cc Show resolved Hide resolved
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
src/core/search/search.cc Outdated Show resolved Hide resolved
chakaz
chakaz previously approved these changes May 7, 2023
Copy link
Collaborator

@chakaz chakaz left a comment

Choose a reason for hiding this comment

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

Very rapid progress, good job! :)

src/core/search/search.cc Outdated Show resolved Hide resolved
src/server/search_family.cc Outdated Show resolved Hide resolved
src/server/search_family.cc Outdated Show resolved Hide resolved
src/server/search_family.cc Show resolved Hide resolved
class ConnectionContext;

class SearchFamily {
static void FtCreate(CmdArgList args, ConnectionContext* cntx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

100% your call, just a suggestion. As always, feel free to overrule :)

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

dranikpg commented May 8, 2023

Fixed comments

@dranikpg dranikpg requested a review from chakaz May 8, 2023 12:03
@dranikpg dranikpg marked this pull request as ready for review May 8, 2023 12:24
@dranikpg dranikpg merged commit c3dc05a into dragonflydb:main May 9, 2023
6 checks passed
@dranikpg dranikpg deleted the parser-basic-brute branch May 13, 2023 13:03
romange pushed a commit that referenced this pull request Jun 1, 2023
Basic search (ft.create & ft.search)
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.

None yet

3 participants