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

ISPN-14620 ZREM, ZREMRANGEBYLEX, ZREMRANGEBYSCORE, ZREMRANGEBYRANK #11213

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

karesti
Copy link
Contributor

@karesti karesti commented Aug 14, 2023

@karesti karesti requested a review from jabolina August 14, 2023 11:12
Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

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

Just a few questions.

* @return the number of removed elements
*/
public CompletionStage<Long> removeAll(K key, V min, boolean includeMin, V max, boolean includeMax) {
return removeAll(key, min, includeMin, max, includeMax, LEX);
Copy link
Member

Choose a reason for hiding this comment

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

From the tests, null values for min/max mean some sort of "unbounded" remove? Maybe we should call that out in the method Java doc. Otherwise, mistakenly passing a null would remove the elements.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the RESP side, it seems the values are supposed to be non-null. Do we include a check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

values are non null, but can be -inf +inf or + -. in these cases, we provide null values to remove from head to tail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the api in the multimap is not awesome, but it's an implementation api, not public one. if we promote this apis, we need to come into what do we really want to support from infinispan hotrod

* The number of members removed from the sorted set, not including non-existing members.
*
* @since 15.0
* @see <a href="https://redis.io/commands/zmscore/">Redis Documentation</a>
Copy link
Member

Choose a reason for hiding this comment

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

Linking another command.

@karesti
Copy link
Contributor Author

karesti commented Aug 16, 2023

@jabolina I fixed the ZREM and added the null parameters explanation

@jabolina jabolina merged commit fb36f5b into infinispan:main Aug 16, 2023
3 of 4 checks passed
@jabolina
Copy link
Member

Thanks, @karesti!

@karesti karesti deleted the ISPN-14620-ZREM-Commands branch August 16, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants