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

Bugfix - Enumerating collection with documents expiring causes early return #422

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

jrpavoncello
Copy link
Contributor

Addresses bug found in #421 i.e. enumerating a collection from a search where the documents were expiring causes an early return resulting in less documents than expected.

@slorello89 Side note - I noticed that the non-generic SearchResponse would return an empty Document i.e. an empty dictionary documentHash, akin to returning null. But the generic SearchResponse<T> just skips the document instead. In the same vein, AggregationResult and AggregationResult<T> also seem to have a general pattern of returning null to their callers (whether internal or external to this package). This seemed like an unexpected functional difference between these classes that all handle a RedisReplys so I wanted to call it to your attention for potential future review since these would be out of scope of a patch.

…xpiring can result in an early return with less documents than expected in the result
@jrpavoncello jrpavoncello force-pushed the bugfix/EnumeratingExpired branch from aaa5f58 to 00342ba Compare November 26, 2023 06:04
@jrpavoncello
Copy link
Contributor Author

@slorello89 Hey Steve, I wasn't sure if you got notifications from PR descriptions so here's an extra ping if not :) I wanted to get your thoughts on the "side note" mentioned there before publishing this PR, I found some stuff that is probably out of scope for this change.

@slorello89 slorello89 marked this pull request as ready for review November 27, 2023 13:16
@slorello89
Copy link
Member

Hi @jrpavoncello - AggregationResults are a bit different in that they will actually return null items based on what's in the pipeline:

127.0.0.1:6379> FT.AGGREGATE catalogentry-idx * LIMIT 0 10
 1) (integer) 1
 2) (empty array)
 3) (empty array)
 4) (empty array)
 5) (empty array)
 6) (empty array)
 7) (empty array)
 8) (empty array)
 9) (empty array)
10) (empty array)
11) (empty array)

WRT to the non-generic version of SearchResponse, I think that I left that there in case someone wanted a more raw version of the Search Response someday, which wasn't a need that ever materialized, so realistically it's probably dead code which should be removed at some point.

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks for submitting this @jrpavoncello

@slorello89 slorello89 merged commit c87f2da into redis:main Nov 27, 2023
1 check passed
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