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

Add binary string support for java client for sorted set commands #1648

Closed
wants to merge 0 commits into from

Conversation

asheryerm
Copy link
Collaborator

@asheryerm asheryerm commented Jun 25, 2024

Add Java client support for GlideString (binary safe string) in the commands: zrevrank, zrevrankWithScore,zremrangebyscore, zremrangebylex, zrange, zrangestore, zrandmember, zlexcounter, zinterstore, zinter, zdiff, zcount, zadd.

@asheryerm asheryerm requested a review from a team as a code owner June 25, 2024 11:58
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Add IT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of duplicating all classes, consider adding new method toBinaryArgs to all existing. That will cause smaller changes.
Update docs in SortedSetBaseCommands.java if you do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I didn't create one class that supports toArgs and toArgsBinary is because this makes the usage more implicit. For example, each class will need a constructor with regular strings and a constructor with GlideStrings. If the user uses the GlideString constructor and then calls toArgs instead of toArgsBinary he could get unexpected results. So I think it is better to separate the logic.

Of course there tradeoffs and both implementations have their pros and cons.

@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Jun 26, 2024
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Too many changes for 1 PR
Try to split

@asheryerm
Copy link
Collaborator Author

Too many changes for 1 PR Try to split

The reason I had to add changes to this PR is because I needed functions/interfaces that I created in the original PR to implement z* commands.

The reason I didn't create one class that supports toArgs and toArgsBinary is because this makes the usage more implicit. For example, each class will need a constructor with regular strings and a constructor with GlideStrings. If the user uses the GlideString constructor and then calls toArgs instead of toArgsBinary he could get unexpected results. So I think it is better to separate the logic.

@asheryerm asheryerm changed the title Add binary more for zrevrank, zrevrankWithScore,zremrangebyscore, zre… Add binary string support for java client for sorted set commands Jul 1, 2024
@asheryerm
Copy link
Collaborator Author

Added IT

@Yury-Fridlyand
Copy link
Collaborator

IT are added, but I can't validate anything above that. The PR is too huge.

MAX;

public GlideString[] toArgs() {
return new GlideString[] {gs(AGGREGATE_REDIS_API), gs(toString())};
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does toString() do here? Same in String implementation

arguments = concatenateArrays(arguments, new GlideString[] {gs("REV")});
}

if (rangeQuery.getLimit() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use ArgsBuilder for smaller code footprint

@@ -139,11 +157,12 @@ public GlideString[] toArgs() {
keys.add(entry.getLeft());
weights.add(entry.getRight());
}
argumentsList.add(GlideString.of(keys.size()));

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use ArgsBuilder - this will reduce the code overhead you have here

*
* @return GlideString[]
*/
public GlideString[] toArgsBinary() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Function public GlideString[] toArgsBinary() could be implemented using this one-liner:

public GlideString[] toArgsBinary() { return new ArgsBuilder().add(toArgs()).toArray(); }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants