Skip to content

Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802]#5511

Merged
GuyAv46 merged 3 commits intomasterfrom
guyav-improve_reduceres
Jan 19, 2025
Merged

Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802]#5511
GuyAv46 merged 3 commits intomasterfrom
guyav-improve_reduceres

Conversation

@GuyAv46
Copy link
Collaborator

@GuyAv46 GuyAv46 commented Jan 15, 2025

Describe the changes in the pull request

Improve the default value of reducers (when all the values in the group are missing)

Which additional issues this PR fixes

  1. MOD-7786
  2. MOD-7787
  3. MOD-7789
  4. MOD-7800
  5. MOD-7802

Reducers this PR modified

  1. Quantile
  2. First Value
  3. Min/Max
  4. Sum/Average
  5. Standard Deviation

Mark if applicable

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

@GuyAv46 GuyAv46 requested a review from alonre24 January 15, 2025 10:21
@GuyAv46 GuyAv46 enabled auto-merge January 15, 2025 10:24
@GuyAv46 GuyAv46 added the bug label Jan 15, 2025
@GuyAv46 GuyAv46 changed the title Improve reducers logic - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Jan 15, 2025
@GuyAv46 GuyAv46 changed the title Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Improve reducers on no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Jan 15, 2025
@GuyAv46 GuyAv46 changed the title Improve reducers on no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] Jan 15, 2025
@codecov
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.20%. Comparing base (41c1e8e) to head (ef768f6).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5511      +/-   ##
==========================================
+ Coverage   87.18%   87.20%   +0.01%     
==========================================
  Files         196      196              
  Lines       35091    35108      +17     
==========================================
+ Hits        30595    30615      +20     
+ Misses       4496     4493       -3     

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

Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Let's just add the new behaviour to docs

Comment on lines +745 to +747
env.expect(*query('FIRST_VALUE', 3, '@missing', 'BY', '@n')).equal([1, ['res', None]])
env.expect(*query('FIRST_VALUE', 3, '@missing', 'BY', '@missing')).equal([1, ['res', None]])
env.expect(*query('FIRST_VALUE', 1, '@missing')).equal([1, ['res', None]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why here we are expecting Python None, but in other reducers it was nan? Is there any actual intentional difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nan is for numeric reducers, while None (or actually NULL, which redispy interprets as None) is for value reducers.
Both, of course, are in the case where no value was found for the group

@GuyAv46 GuyAv46 added this pull request to the merge queue Jan 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2025
@GuyAv46 GuyAv46 added this pull request to the merge queue Jan 19, 2025
Merged via the queue into master with commit 44861ce Jan 19, 2025
10 checks passed
@GuyAv46 GuyAv46 deleted the guyav-improve_reduceres branch January 19, 2025 16:30
redisearch-backport-pull-request bot pushed a commit that referenced this pull request Jan 19, 2025
…OD-7800, MOD-7802] (#5511)

* improve reducers logic

* added tests

* rename test argument

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

github-merge-queue bot pushed a commit that referenced this pull request Jan 19, 2025
…789, MOD-7800, MOD-7802] (#5526)

Improve reducers no-value behavior - [MOD-7786, MOD-7787, MOD-7789, MOD-7800, MOD-7802] (#5511)

* improve reducers logic

* added tests

* rename test argument

(cherry picked from commit 44861ce)

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