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? # for a free GitHub account to open an issue and contact its maintainers and the community.

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants