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.

Sign up for GitHub

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

Jump to bottom

fix #2517 Don't compare if p_pref is empty #2521

Merged
merged 9 commits into from
Jan 31, 2022
Merged

fix #2517 Don't compare if p_pref is empty #2521

merged 9 commits into from
Jan 31, 2022

Conversation

gkorland
Copy link
Contributor

@gkorland gkorland commented Jan 20, 2022 •
edited by ashtul
Loading

related to #1776

@gkorland gkorland changed the title Don't compare if p_pref is empty fix #2517 Don't compare if p_pref is empty Jan 20, 2022
@gkorland gkorland linked an issue Jan 20, 2022 that may be closed by this pull request
contains() with an empty string argument leaves Redis hanging at CPU 100% indefinitely #2517
Closed
@codecov
Copy link

codecov bot commented Jan 20, 2022 •
edited
Loading

Codecov Report

Merging #2521 (b971450) into master (9b6e1ae) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2521      +/-   ##
==========================================
+ Coverage   80.92%   80.94%   +0.02%     
==========================================
  Files         175      175              
  Lines       26513    26515       +2     
==========================================
+ Hits        21455    21462       +7     
+ Misses       5058     5053       -5
Impacted Files Coverage Δ
src/aggregate/functions/string.c 99.54% <100.00%> (+<0.01%) ⬆️
src/inverted_index.c 75.07% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b6e1ae...b971450. Read the comment docs.

@ashtul ashtul requested a review from oshadmi January 30, 2022 10:13
oshadmi
oshadmi reviewed Jan 30, 2022
View reviewed changes
Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

Small comment

src/aggregate/functions/string.c Outdated Show resolved Hide resolved
@gkorland gkorland marked this pull request as ready for review January 30, 2022 21:17
@ashtul ashtul requested a review from oshadmi January 31, 2022 08:26
oshadmi
oshadmi previously approved these changes Jan 31, 2022
View reviewed changes
Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👍🏼

Regarding documentation, it says:
Return the number of occurrences of s2 in s1, 0 otherwise.

So it can remain as-is
But we can also specifically mention an empty s2 will return "length of s1 + 1"
So users avoid unpleasant surprises such as infinite loops.

@ashtul ashtul merged commit 54113e7 into master Jan 31, 2022
@ashtul ashtul deleted the gkorland-emptystring-loop branch January 31, 2022 14:02
ashtul pushed a commit that referenced this pull request Jan 31, 2022
@gkorland @ashtul
fix #2517 Don't compare if p_pref is empty (#2521)
7e846c2 
* don't compare if p_pref is empty

* if empty return len+1. add tests

* Update src/aggregate/functions/string.c

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* do not init num

* fix test

* documentation

Co-authored-by: Ariel Shtul <ashtul@gmail.com>
Co-authored-by: Ariel Shtul <ariel.shtul@redislabs.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
(cherry picked from commit 54113e7)
ashtul pushed a commit that referenced this pull request Feb 1, 2022
@gkorland @ashtul
fix #2517 Don't compare if p_pref is empty (#2521)
22eb917 
* don't compare if p_pref is empty

* if empty return len+1. add tests

* Update src/aggregate/functions/string.c

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* do not init num

* fix test

* documentation

Co-authored-by: Ariel Shtul <ashtul@gmail.com>
Co-authored-by: Ariel Shtul <ariel.shtul@redislabs.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
(cherry picked from commit 54113e7)
ashtul pushed a commit that referenced this pull request Feb 3, 2022
@gkorland
fix #2517 Don't compare if p_pref is empty (#2521)
c95b7f6 
* don't compare if p_pref is empty

* if empty return len+1. add tests

* Update src/aggregate/functions/string.c

Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* do not init num

* fix test

* documentation

Co-authored-by: Ariel Shtul <ashtul@gmail.com>
Co-authored-by: Ariel Shtul <ariel.shtul@redislabs.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
(cherry picked from commit 54113e7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@oshadmi oshadmi oshadmi approved these changes

@MeirShpilraien MeirShpilraien Awaiting requested review from MeirShpilraien

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

contains() with an empty string argument leaves Redis hanging at CPU 100% indefinitely
3 participants
@gkorland @oshadmi @ashtul