Skip to content

Fix FT.Search Limit argument and add CountOnly argument for limit 0 0 #3338

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

Merged
merged 3 commits into from
Apr 15, 2025

Conversation

ofekshenawa
Copy link
Collaborator

Adding a new boolean flag, CountOnly, to the FTSearchOptions struct to decouple count-only behavior from the default limit parameters.
Previously, it wasn't possible to input 0 0 for LimitOffset and Limit Now, with the CountOnly flag, users can explicitly signal a count-only query.
When CountOnly is set to true, the FT.SEARCH command builder appends LIMIT 0 0, ensuring that only the total count of matching documents is returned. When CountOnly is false, the provided LimitOffset and Limit values are used as usual.

@ofekshenawa ofekshenawa requested a review from ndyakov April 8, 2025 07:33
@ndyakov ndyakov marked this pull request as ready for review April 8, 2025 19:39
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Left comment regarding naming, I do think maybe CountsOnly will be the best we can do right now:

    CountOnly  bool // when true, sends LIMIT 0 0 to only get count

@elena-kolevska
Copy link
Contributor

elena-kolevska commented Apr 14, 2025

@ofekshenawa when this is merged please ping the docs team to create a new doctests example with a CountOnly example, or try updating it yourself and then ask their help with marking it up so it shows up properly in the docs.

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ndyakov
Copy link
Member

ndyakov commented Apr 15, 2025

@andy-stark-redis bringing this to your attention. Can we please add a doctest for this case?

@ndyakov ndyakov merged commit 6e07177 into master Apr 15, 2025
16 checks passed
@andy-stark-redis
Copy link
Contributor

@andy-stark-redis bringing this to your attention. Can we please add a doctest for this case?

@ndyakov Sure, no problem :-)

@elena-kolevska elena-kolevska deleted the ft-search-fix-limit branch April 17, 2025 15:11
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.

4 participants