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

query object is not outputing the TIMEOUT argument in case of 0 ( 0 means unlimited for RediSearch TIMEOUT parameter ) #2839

Closed
filipecosta90 opened this issue Jul 10, 2023 · 1 comment
Labels
bug Bug

Comments

@filipecosta90
Copy link

Version: 4.6.0 or any above 4.4.1 given the timeout parameter was introduced there (https://github.com/redis/redis-py/releases/tag/v4.4.1)

Description: I was checking why we where not seeing the timeout parameter on a RediSearch query monitor with timeout(0) which means unlimited execution time on RediSearch.
Given the following:

q = (
            Query(
                f"{prefilter_condition}=>[KNN $K @vector $vec_param EF_RUNTIME $EF AS vector_score]"
            )
            .sort_by("vector_score", asc=False)
            .paging(0, top)
            .return_fields("vector_score")
            .dialect(2)
            .timeout(0)
        )

notice that on the output of q.get_args() we're missing the TIMEOUT 0:

['*=>[KNN $K @vector $vec_param EF_RUNTIME $EF AS vector_score]', 'RETURN', 1, 'vector_score', 'SORTBY', 'vector_score', 'DESC', 'DIALECT', 2, 'LIMIT', 0, 100]

checking the code we see that the integer 0 will be interpreted as False on the condition https://github.com/redis/redis-py/blob/master/redis/commands/search/query.py#L197

if self._timeout:
   args += ["TIMEOUT", self._timeout]
@chayim chayim added the bug Bug label Jul 13, 2023
meiravgri added a commit to meiravgri/redis-py that referenced this issue Sep 12, 2023
In RediSearch query timeout = 0 means unlimited timeout.
In the current implementation, the query timeout is only updated `if timeout` which in case of 0, translates to false.
Since the default timeout of the `query` class is None, replacing the condition with `if self._timeout is not None` will also work for 0.
If the parameter is not a positive integer, redis server will raise an exception.

related issue: redis#2928 redis#2839
@filipecosta90
Copy link
Author

To be fixed by #2934

dvora-h pushed a commit that referenced this issue Sep 14, 2023
* Support query timeout = 0

In RediSearch query timeout = 0 means unlimited timeout.
In the current implementation, the query timeout is only updated `if timeout` which in case of 0, translates to false.
Since the default timeout of the `query` class is None, replacing the condition with `if self._timeout is not None` will also work for 0.
If the parameter is not a positive integer, redis server will raise an exception.

related issue: #2928 #2839

* added a test to quety.timeout(0)

* raise an exception if query TIMEOUT is a non negative integer

* fixed the query test to catach AttributeError

* Moved validating timeout to timeout()

Raise the exception when the timeout is set instead of when the query is performed

* updates test to catch the exception when query.timeout() is called

* Update redis/commands/search/query.py

Co-authored-by: GuyAv46 <[email protected]>

* Revert "Update redis/commands/search/query.py"

This reverts commit fb2b710.

* Revert "updates test to catch the exception when query.timeout() is called"

This reverts commit 6590130.

* Revert "Moved validating timeout to timeout()"

This reverts commit 7a020bd.

* Revert "fixed the query test to catach AttributeError"

This reverts commit 25d4ddf.

* Revert "raise an exception if query TIMEOUT is a non negative integer"

This reverts commit 3fb2c68.

---------

Co-authored-by: GuyAv46 <[email protected]>
@dvora-h dvora-h closed this as completed Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

No branches or pull requests

3 participants