GEODE-9378: Implement ZRANGEBYSCORE #6700

ringles · 2021-07-15T15:52:47Z

Addresses GEODE-9378 to implement the ZRANGEBYSCORE command for Geode Radish, the compatible-with-Redis API.

dschneider-pivotal · 2021-07-15T21:13:06Z

...c/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java

+  public List<byte[]> zrangebyscore(RedisKey key, SortedSetRangeOptions rangeOptions,
+      boolean withScores) {
+    return stripedExecute(key,
+        () -> getRedisSortedSet(key, true).zrangebyscore(rangeOptions, withScores));


I noticed you are calling getRedisSortedSet with updatesStats set to true. This seems reasonable. But I also noticed that a bunch of other methods on this class (zadd, zincrby, zrange, zrem) call it with false. Do you know when to set it to true vs false?

I believe that Redis' approach is to update the stats for commands that don't update data. In your sample list, zrange actually does call it with true.

I see ZRANGE calling it with true. In general, operations that read data update stats, operations that change data do not. So zadd, zrem, zincrby change data and therefore don't update stats.

dschneider-pivotal · 2021-07-15T21:20:50Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+    List<byte[]> result = new ArrayList<>();
+    AbstractOrderedSetEntry minEntry =
+        new DummyOrderedSetEntry(rangeOptions.getMinDouble(), rangeOptions.isMinExclusive(), true);
+    long minIndex = scoreSet.indexOf(minEntry);


why is minIndex and maxIndex typed as long? indexOf returns an int and later we cast it to an int. Seems like if you typed it as "int" you could get rid of the cast later.

They're typed as long because they're essentially the same code as in ZCOUNT. But it makes more sense for them to be int here, as you say.

dschneider-pivotal · 2021-07-15T21:35:39Z

.../src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java

+        count = Integer.MAX_VALUE;
+      }
+    } else {
+      throw new Exception();


can you make this a more specific exception? When the higher level catches Exception it could be catching all kinds of things (like NullPointerException) and reporting them as something else

IllegalArgumentException seems to fit. Done.

jdeppe-pivotal · 2021-07-16T14:54:44Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+
+    // Count 1 <= score <= 1
+    assertThat(jedis.zrangeByScore(KEY, score, score))
+        .containsExactlyInAnyOrderElementsOf(map.keySet());


I think this should also be using containsExactly since the order of the returned values should be lexicographically sorted.

Tweaked; unfortunately the map.keySet() isn't in the order we want, but it's fixable.

jdeppe-pivotal · 2021-07-16T15:08:15Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractZRangeByScoreIntegrationTest implements RedisIntegrationTest {


There are a lot of boundary cases tested, but please would you also add a basic test for exclusivity.

Could you also add tests to confirm the behaviour of ZRANGEBYSCORE with multiple instances of WITHSCORES and LIMIT, and with incorrectly formatted LIMIT arguments?

Hope the new one is the kind of thing you have in mind...

jdeppe-pivotal · 2021-07-16T15:13:08Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+      if (skip < offset) {
+        skip++;
+        continue;
+      }


You could avoid use of skip here and just count down on offset:

if (offset > 0) { offset--; continue; }

Nice; I think I can do that with 'count' as well!

DonalEvans · 2021-07-16T16:45:39Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+  }
+
+  @Test
+  public void shouldReturnCount_givenRangeIncludingScore() {


This test name should probably be "shouldReturnMember_givenRangeIncludingScore" or something similar.

Yeah, missed one.

DonalEvans · 2021-07-16T16:46:25Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+  public void shouldReturnZero_givenMinGreaterThanMax() {
+    jedis.zadd(KEY, 1, "member");
+
+    // Count +inf <= score <= -inf


This comment (and others in this class) is a little misleading, as these tests aren't returning a count, but rather a range.

Yeah, thought I got all those. Quick fix.

DonalEvans · 2021-07-16T16:48:39Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+
+    // Count -inf < score <= +inf
+    assertThat(jedis.zrangeByScore(KEY, "(-inf", "+inf"))
+        .containsExactlyElementsOf(Arrays.asList("member2", "member3"));


This can be simplified to .containsExactly("member2", "member3"). This also applies to other places in this test where Arrays.asList() and containsExactlyElementsOf() are used together.

Got a few of them earlier, all done now!

DonalEvans · 2021-07-16T16:54:04Z

...is-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RedisCommandType.java

-  ZREVRANGE(new ZRevRangeExecutor(), SUPPORTED, new MinimumParameterRequirements(4)
-      .and(new MaximumParameterRequirements(5, ERROR_SYNTAX))),


This change should be reverted as ZREVRANGE should return a syntax error if more than 5 arguments are passed to it.

DonalEvans · 2021-07-16T17:07:31Z

...compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java

+    Iterator<AbstractOrderedSetEntry> entryIterator =
+        scoreSet.getIndexRange(minIndex, maxIndex, false);
+    if (rangeOptions.hasLimit()) {
+      count = rangeOptions.getCount();
+      offset = rangeOptions.getOffset();
+    }


Rather than getting the range iterator and then skipping to the offset, might it be neater to add the offset to minIndex and then retrieve the range iterator? Similarly, I think you should be able to pass Math.min(count, maxIndex - minIndex) as the second argument of getIndexRange() so that you get an iterator that's automatically the right size. Then you can just iterate the entire length and return that with no further checks needed.

Short answer: Yes.
Long answer: Yes, and we can eliminate another variable, too.

DonalEvans · 2021-07-16T17:29:59Z

...org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByScoreIntegrationTest.java

+
+import org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractZRangeByScoreIntegrationTest implements RedisIntegrationTest {


Could you also add tests to confirm the behaviour of ZRANGEBYSCORE with multiple instances of WITHSCORES and LIMIT, and with incorrectly formatted LIMIT arguments?

DonalEvans · 2021-07-16T17:51:12Z

.../src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java

+      offset = narrowLongToInt(bytesToLong(commandElements.get(commandIndex + 1)));
+      count = narrowLongToInt(bytesToLong(commandElements.get(commandIndex + 2)));


With an improperly provided LIMIT argument (one or both of offset or count not supplied) this code will throw an ArrayIndexOutOfBoundsException which is not handled, so no syntax error will be returned to the client.

Tests and fixes added for these.

DonalEvans · 2021-07-16T17:53:25Z

...ible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java

  public static final byte[] bRADISH_DUMP_HEADER = stringToBytes("RADISH");

+  @MakeImmutable
+  public static final byte[] bRADISH_WITHSCORES = stringToBytes("WITHSCORES");


There is already a byte array constant for WITHSCORES further up in this class (line 180), so this one is not necessary.

Good catch, fixed!

DonalEvans · 2021-07-16T17:58:01Z

...ible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java

+  public static final byte[] bRADISH_WITHSCORES = stringToBytes("WITHSCORES");
+
+  @MakeImmutable
+  public static final byte[] bRADISH_LIMIT = stringToBytes("LIMIT");


Could this be renamed "bLIMIT" to match the naming of other byte array constants used in executors, and moved further up in this class to be with the other executor-related constants?

Also corrected!

jdeppe-pivotal

Nice work on this!

ringles added the redis Issues related to the geode-for-redis module label Jul 15, 2021

ringles requested a review from DonalEvans July 15, 2021 16:56

ringles added 5 commits July 15, 2021 14:06

Basic plumbing and tests

d429ced

Extract method, fix some warnings

2cb0705

Update for new getIndexRange() signature

ab555ca

Fix unused vars/class class and de-hardcode arguments

c76a72d

Refactor SortedSetRangeOptions, fix some ZRANGEBYSCORE parsing issues

67b5b7d

dschneider-pivotal reviewed Jul 15, 2021

View reviewed changes

Review feedback and some polish

8d60d6c

jdeppe-pivotal requested changes Jul 16, 2021

View reviewed changes

DonalEvans requested changes Jul 16, 2021

View reviewed changes

ringles added 11 commits July 16, 2021 14:04

Add requested test & refactor, remove unneeded variable

9656679

Fix comments & test name, revert inadvertent change

6bdf5c1

Remove unneeded variables/iteration, add test for improper LIMIT args

28c41c8

Test multiple WITHSCORES/LIMIT clauses

7135797

Remove unnecessary limiting code

0e7976f

Logically group logical conditions

36836db

Strip trailing '.0' from doubles like Redis does

1543363

Review feedback applied

9db0538

Fix test to match new behavior

e0ca9c3

Update ZADD CH to handle stripped '.0' on scores

7d32997

Simplify remaning tests

37455b2

ringles marked this pull request as ready for review July 19, 2021 19:09

ringles requested review from nonbinaryprogrammer and upthewaterspout as code owners July 19, 2021 19:09

DonalEvans approved these changes Jul 19, 2021

View reviewed changes

jdeppe-pivotal approved these changes Jul 20, 2021

View reviewed changes

ringles merged commit 0726d5a into apache:develop Jul 20, 2021


		import org.apache.geode.redis.RedisIntegrationTest;

		public abstract class AbstractZRangeByScoreIntegrationTest implements RedisIntegrationTest {

		ZREVRANGE(new ZRevRangeExecutor(), SUPPORTED, new MinimumParameterRequirements(4)
		.and(new MaximumParameterRequirements(5, ERROR_SYNTAX))),

		offset = narrowLongToInt(bytesToLong(commandElements.get(commandIndex + 1)));
		count = narrowLongToInt(bytesToLong(commandElements.get(commandIndex + 2)));

GEODE-9378: Implement ZRANGEBYSCORE #6700

GEODE-9378: Implement ZRANGEBYSCORE #6700

Uh oh!

Conversation

ringles commented Jul 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeppe-pivotal Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeppe-pivotal Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdeppe-pivotal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jdeppe-pivotal Jul 16, 2021 •

edited

Loading

jdeppe-pivotal Jul 16, 2021 •

edited

Loading