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.

Sign up for GitHub

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

Jump to bottom

fix(search_family): Fix LOAD fields parsing in the FT.AGGREGATE and FT.SEARCH commands #4012

Merged
merged 6 commits into from
Nov 25, 2024
Merged

fix(search_family): Fix LOAD fields parsing in the FT.AGGREGATE and FT.SEARCH commands #4012

merged 6 commits into from
Nov 25, 2024

Conversation

BagritsevichStepan
Copy link
Contributor

fixes #3989

@BagritsevichStepan BagritsevichStepan self-assigned this Oct 29, 2024
@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch from 73c5897 to 2170419 Compare October 30, 2024 15:21
@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch from 2170419 to 1ea8bf4 Compare October 30, 2024 15:35
@BagritsevichStepan BagritsevichStepan requested review from adiholden and removed request for dranikpg October 31, 2024 14:07
@romange romange self-requested a review November 3, 2024 09:30
dranikpg
dranikpg previously approved these changes Nov 3, 2024
View reviewed changes
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

It feels a bit to complicated for the task. Feel free to change Schema struct to be easier to use, feel free to simplify the flow. I'd also suggest to simplify the SearchField struct

src/server/search/doc_index.h Outdated Show resolved Hide resolved
src/server/search/doc_index.h Outdated Show resolved Hide resolved
src/server/search/doc_index.h Outdated Show resolved Hide resolved
src/server/search/doc_index.h Outdated Show resolved Hide resolved
@dranikpg
Copy link
Contributor

dranikpg commented Nov 3, 2024

I'll be offline the next 3 days, if you're in a hurry you can ping anyone else 🙂 (for example Kostas is always happy to review 😆)

@BagritsevichStepan
Copy link
Contributor Author

BagritsevichStepan commented Nov 3, 2024 •
edited
Loading

It feels a bit to complicated for the task. Feel free to change Schema struct to be easier to use, feel free to simplify the flow. I'd also suggest to simplify the SearchField struct

Thanks for review, but I don't understand what you mean by "Feel free to change Schema struct to be easier to use"?

I see that I can make SearchField much simpler. But what can be improved in the Schema class? Just remove LookupIdentifier that I added?

@romange romange dismissed dranikpg’s stale review November 3, 2024 10:22

need improvements

@romange
Copy link
Collaborator

romange commented Nov 3, 2024

@BagritsevichStepan I will continue the review and I agree with the sentiment of Vlad.
Please do the first round of improvements and then we will see if it can be simplified further.

@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch from 1ea8bf4 to 95d86c2 Compare November 3, 2024 15:47
@BagritsevichStepan BagritsevichStepan requested review from romange and removed request for adiholden and romange November 3, 2024 15:49
@BagritsevichStepan
Copy link
Contributor Author

BagritsevichStepan commented Nov 3, 2024 •
edited
Loading

Do not review, I'm refactoring another part of code

romange
romange reviewed Nov 3, 2024
View reviewed changes
src/core/search/search.h Outdated Show resolved Hide resolved
src/server/search/doc_index.h Show resolved Hide resolved
src/server/search/doc_index.h Outdated Show resolved Hide resolved
src/server/search/doc_index.h Outdated Show resolved Hide resolved
src/server/search/search_family.cc

StringOrView name = StringOrView::FromString(std::string{str});
if (parser->Check("AS")) {
load_fields->value().emplace_back(name, true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you add aliases as strings. Where do you benefit from string_view, in other words, why using StringOrView types?

Copy link
Contributor Author

@BagritsevichStepan BagritsevichStepan Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are storing field names as strings in the indexes (see Schema class here). At some stage we are joining load_fields and already indexed fields (from Schema) to one container. So the problem is that we can move strings from load_fields, but not strings from Schema. That's why we should use string_views

@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch 2 times, most recently from a62cf79 to 8375d3f Compare November 19, 2024 11:26
dranikpg
dranikpg previously approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Noticeably shorter than the previous version

src/server/search/search_family.cc Outdated Show resolved Hide resolved
src/server/search/doc_index.cc Outdated Show resolved Hide resolved
@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch from f384ec6 to 3b845f1 Compare November 20, 2024 04:29
@BagritsevichStepan BagritsevichStepan force-pushed the df.search/fix-load-option-arguments-parsing branch from 3b845f1 to 75b4df8 Compare November 25, 2024 09:09
@BagritsevichStepan BagritsevichStepan enabled auto-merge (squash) November 25, 2024 09:32
@BagritsevichStepan BagritsevichStepan merged commit 2b3c182 into dragonflydb:main Nov 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BorysTheDev BorysTheDev BorysTheDev approved these changes

@romange romange Awaiting requested review from romange

@dranikpg dranikpg Awaiting requested review from dranikpg

Assignees

@BagritsevichStepan BagritsevichStepan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong LOAD fields parsing in the FT.AGGREGATE and FT.SEARCH commands for JSON data
4 participants
@BagritsevichStepan @dranikpg @romange @BorysTheDev