Skip to content

Add integration tests for Redis 8 behavior changes in Redis Search #3337

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 10 commits into from
Apr 16, 2025

Conversation

ofekshenawa
Copy link
Collaborator

This commit adds a suite of integration tests to validate the expected behavior changes and bug fixes introduced for Redisearch in Redis Community Edition 8. These tests cover several areas including query syntax, reducer default values, scoring defaults, alias validations, timeout behavior, and stop word handling.

IMPORTANT: This PR adds tests only to verify that the changes occur correctly. It does not implement any modifications to Redisearch itself.

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.

I don't think we should allow LIMIT 0 0 from the ft.search... We can introduce additional option to note that the execution should only count the results (and add LIMIT 0 0) but enabling it for all ft.search executions will break the default behaviour without setting the LimitOffset and Limit options since 0 is the default value for those integer. WDYT @ofekshenawa ?

@ofekshenawa ofekshenawa force-pushed the os-test-behvioral-changes-in-search-on-redis-8 branch from 5dd9515 to ec1b320 Compare April 7, 2025 14:14
@ofekshenawa ofekshenawa force-pushed the os-test-behvioral-changes-in-search-on-redis-8 branch from 51ec981 to 59e017e Compare April 8, 2025 07:42
@ndyakov
Copy link
Member

ndyakov commented Apr 8, 2025

blocked by #3338

@ofekshenawa ofekshenawa marked this pull request as ready for review April 15, 2025 16:26
@ofekshenawa ofekshenawa requested a review from ndyakov April 15, 2025 16:26
ndyakov
ndyakov previously approved these changes Apr 15, 2025
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.

Thank you for addressing the issues and adding the needed tests. I think it is OK to be merged as it is, let's briefly discuss how we would like to handle the deprecations.

@ndyakov ndyakov self-requested a review April 16, 2025 12:55
@ndyakov ndyakov merged commit 7b9bd6c into master Apr 16, 2025
16 checks passed
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.

2 participants