Skip to content

Fix legacy geofilter leak - [MOD-8568]#5523

Merged
GuyAv46 merged 4 commits intomasterfrom
guyav-fix_legacy_geofilter_leak_master
Jan 22, 2025
Merged

Fix legacy geofilter leak - [MOD-8568]#5523
GuyAv46 merged 4 commits intomasterfrom
guyav-fix_legacy_geofilter_leak_master

Conversation

@GuyAv46
Copy link
Collaborator

@GuyAv46 GuyAv46 commented Jan 19, 2025

Describe the changes in the pull request

Fix a bug when a query has multiple GEOFILTER filters. Instead of applying them all (like we do with the legacy numeric filters), we override the single filter pointer, causing a leak and also yielding wrong results.

This PR is parallel to #5516 and targeted to master and 8.0 branches.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@GuyAv46 GuyAv46 requested a review from alonre24 January 19, 2025 09:39
@GuyAv46 GuyAv46 marked this pull request as ready for review January 19, 2025 11:34
@GuyAv46 GuyAv46 marked this pull request as draft January 19, 2025 11:34
@codecov
Copy link

codecov bot commented Jan 19, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.21%. Comparing base (14da65a) to head (d089d4a).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/aggregate/aggregate_request.c 80.95% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5523   +/-   ##
=======================================
  Coverage   87.21%   87.21%           
=======================================
  Files         196      196           
  Lines       35226    35226           
=======================================
+ Hits        30722    30723    +1     
+ Misses       4504     4503    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GuyAv46 GuyAv46 force-pushed the guyav-fix_legacy_geofilter_leak_master branch from 42f02ac to d089d4a Compare January 19, 2025 17:26
@GuyAv46 GuyAv46 marked this pull request as ready for review January 19, 2025 17:27
@GuyAv46 GuyAv46 enabled auto-merge January 19, 2025 17:27
@GuyAv46 GuyAv46 added this pull request to the merge queue Jan 22, 2025
@GuyAv46 GuyAv46 removed this pull request from the merge queue due to a manual request Jan 22, 2025
@GuyAv46 GuyAv46 added this pull request to the merge queue Jan 22, 2025
Merged via the queue into master with commit fdd48ae Jan 22, 2025
13 of 16 checks passed
@GuyAv46 GuyAv46 deleted the guyav-fix_legacy_geofilter_leak_master branch January 22, 2025 13:50
redisearch-backport-pull-request bot pushed a commit that referenced this pull request Jan 22, 2025
* fix bug

* added a test

* fix typo

* fix edge case leak

(cherry picked from commit fdd48ae)
@redisearch-backport-pull-request
Copy link
Contributor

Successfully created backport PR for 8.0:

github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2025
Fix legacy geofilter leak - [MOD-8568] (#5523)

* fix bug

* added a test

* fix typo

* fix edge case leak

(cherry picked from commit fdd48ae)

Co-authored-by: GuyAv46 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants