You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/07/22 18:36:39 UTC
[GitHub] [geode] DonalEvans opened a new pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
DonalEvans opened a new pull request #6716:
URL: https://github.com/apache/geode/pull/6716
- Add ZRANGEBYLEX to supported commands
- Introduce AbstractSortedSetRangeOptions class to handle behaviour
common to -BYSCORE and -BYLEX ranges
- Introduce SortedSetLexRangeOptions to handle parsing lexical ranges
- Rename SortedSetRangeOptions to SortedSetScoreRangeOptions
- Introduce MemberDummyOrderedSetEntry to support getting rank by lex
- Rename DummyOrderedSetEntry to ScoreDummyOrderedSetEntry
- Add unit tests for MemberDummyOrderedSetEntry
- Add integration tests for ZRANGEBYLEX
- Refactor RedisSortedSet.zrangebyscore() to use
SortedSetScoreRangeOptions
- Add missing HitsMissesIntegrationTest for ZRANGEBYSCORE
- Refactor argument parsing in ZRangeByScoreExecutor to allow correct
error behaviour with multiple LIMIT options, one of which is
incorrectly formatted
- Add tests to confirm correct behaviour for ZRANGEBYSCORE for above
case
Authored-by: Donal Evans <do...@vmware.com>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675168845
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java
##########
@@ -33,35 +30,35 @@
public class ZRangeByScoreExecutor extends AbstractExecutor {
@Override
public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
- RedisSortedSetCommands redisSortedSetCommands = context.getSortedSetCommands();
-
List<byte[]> commandElements = command.getProcessedCommand();
- SortedSetRangeOptions rangeOptions;
- boolean withScores = false;
+ SortedSetScoreRangeOptions rangeOptions;
try {
byte[] minBytes = commandElements.get(2);
byte[] maxBytes = commandElements.get(3);
- rangeOptions = new SortedSetRangeOptions(minBytes, maxBytes);
+ rangeOptions = new SortedSetScoreRangeOptions(minBytes, maxBytes);
} catch (NumberFormatException ex) {
return RedisResponse.error(ERROR_MIN_MAX_NOT_A_FLOAT);
}
// Native redis allows multiple "withscores" and "limit ? ?" clauses; the last "limit"
// clause overrides any previous ones
- if (commandElements.size() >= 5) {
- int currentCommandElement = 4;
+ // In order to match the error reporting behaviour of native Redis, the argument list must be
+ // parsed in reverse order. Stop parsing at index = 4, since 0 is the command name, 1 is the
+ // key, 2 is the min and 3 is the max
+ boolean withScores = false;
- while (currentCommandElement < commandElements.size()) {
+ if (commandElements.size() >= 5) {
Review comment:
I plan to do a major refactor of this code as part of the ZREVRANGEBYLEX work, once all the other range commands are implemented, as you're right, there's a lot of duplication and potential for simplification. I think we could probably manage to have a single AbstractZRangeExecutor class, extended by the ZRange, ZRevRange, ZRangeBYScore, ZRevRangeBYScore, ZRangeByLex and ZRevRangeByLex executors that populates an AbstractSortedSetRangeOptions class, extended by SortedSetRankRangeOptions, SortedSetScoreRangeOptions and SortedSetLexRangeOptions classes. Maybe I'm overthinking it though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675823398
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
@MakeImmutable
public static final byte[] bABSTTL = stringToBytes("ABSTTL");
- // AbstractZRangeExecutor
+ // Various ZRangeExecutors
@MakeImmutable
public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
@MakeImmutable
public static final byte[] bLIMIT = stringToBytes("LIMIT");
+ public static final byte bPLUS = SIMPLE_STRING_ID; // +
Review comment:
See https://issues.apache.org/jira/browse/GEODE-9449 and https://issues.apache.org/jira/browse/GEODE-9450
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675825516
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -37,25 +39,32 @@
}
void parseLimitArguments(List<byte[]> commandElements, int commandIndex) {
- if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex - 2), bLIMIT)) {
+ String element = bytesToString(commandElements.get(commandIndex));
+ if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex), bLIMIT)) {
+ System.out.println("DONAL: throwing because " + element + " is not LIMIT");
Review comment:
remove the debug logging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675156262
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
@MakeImmutable
public static final byte[] bABSTTL = stringToBytes("ABSTTL");
- // AbstractZRangeExecutor
+ // Various ZRangeExecutors
@MakeImmutable
public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
@MakeImmutable
public static final byte[] bLIMIT = stringToBytes("LIMIT");
+ public static final byte bPLUS = SIMPLE_STRING_ID; // +
Review comment:
Prior to the refactor that moved constants out of `Coder`, the approach to naming byte/byte array constants was somewhat inconsistent, with some using the "b" prefix and some not. I can't say why this naming convention was originally chosen (commit history shows that it seems to have been present since Geode was first imported to git back in 2015), but when working on https://issues.apache.org/jira/browse/GEODE-9287, I tried to enforce consistency in all the constant names. I think I was overzealous with the single-byte values though, as you're right, many of them don't use the "b" prefix and conceptually it might make more sense to only use it for byte arrays. I can go through and rename the ones are that currently using it so that they're not if you think that'd improve things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675163156
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -636,4 +663,48 @@ public int getSizeInBytes() {
return 0;
}
}
+
+ // Dummy entry used to find the rank of an element with the given member name for lexically
+ // ordered sets
+ static class MemberDummyOrderedSetEntry extends AbstractOrderedSetEntry {
+ boolean isExclusive;
+ boolean isMinimum;
+
+ MemberDummyOrderedSetEntry(byte[] member, double score, boolean isExclusive,
+ boolean isMinimum) {
+ this.member = member;
+ this.scoreBytes = null;
+ this.score = score;
+ this.isExclusive = isExclusive;
+ this.isMinimum = isMinimum;
+ }
+
+ @Override
+ public int compareMembers(byte[] array1, byte[] array2) {
+ int dummyNameComparison = checkDummyMemberNames(array1, array2);
+ if (dummyNameComparison != 0) {
+ return dummyNameComparison;
+ } else {
+ // If not using dummy member names, move on to actual lexical comparison
+ int stringComparison = javaImplementationOfAnsiCMemCmp(array1, array2);
+ if (stringComparison == 0) {
+ if (isMinimum && isExclusive || !isMinimum && !isExclusive) {
Review comment:
Thanks for this! I was sure there must be a neater way of doing that but I wasn't sure what it was. I applied the same change to the constructor of `ScoreDummyOrderedSetEntry` which was the other case you saw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675654618
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java
##########
@@ -33,35 +30,35 @@
public class ZRangeByScoreExecutor extends AbstractExecutor {
@Override
public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
- RedisSortedSetCommands redisSortedSetCommands = context.getSortedSetCommands();
-
List<byte[]> commandElements = command.getProcessedCommand();
- SortedSetRangeOptions rangeOptions;
- boolean withScores = false;
+ SortedSetScoreRangeOptions rangeOptions;
try {
byte[] minBytes = commandElements.get(2);
byte[] maxBytes = commandElements.get(3);
- rangeOptions = new SortedSetRangeOptions(minBytes, maxBytes);
+ rangeOptions = new SortedSetScoreRangeOptions(minBytes, maxBytes);
} catch (NumberFormatException ex) {
return RedisResponse.error(ERROR_MIN_MAX_NOT_A_FLOAT);
}
// Native redis allows multiple "withscores" and "limit ? ?" clauses; the last "limit"
// clause overrides any previous ones
- if (commandElements.size() >= 5) {
- int currentCommandElement = 4;
+ // In order to match the error reporting behaviour of native Redis, the argument list must be
+ // parsed in reverse order. Stop parsing at index = 4, since 0 is the command name, 1 is the
+ // key, 2 is the min and 3 is the max
+ boolean withScores = false;
- while (currentCommandElement < commandElements.size()) {
+ if (commandElements.size() >= 5) {
Review comment:
sounds good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans merged pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #6716:
URL: https://github.com/apache/geode/pull/6716
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675827212
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -37,25 +39,32 @@
}
void parseLimitArguments(List<byte[]> commandElements, int commandIndex) {
- if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex - 2), bLIMIT)) {
+ String element = bytesToString(commandElements.get(commandIndex));
+ if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex), bLIMIT)) {
+ System.out.println("DONAL: throwing because " + element + " is not LIMIT");
Review comment:
the debug logging is in a couple of places
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675670823
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
@MakeImmutable
public static final byte[] bABSTTL = stringToBytes("ABSTTL");
- // AbstractZRangeExecutor
+ // Various ZRangeExecutors
@MakeImmutable
public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
@MakeImmutable
public static final byte[] bLIMIT = stringToBytes("LIMIT");
+ public static final byte bPLUS = SIMPLE_STRING_ID; // +
Review comment:
I can see the danger of having byte arrays that you want to be constants. In fact any array that you want to treat as a constant could be an issue because arrays can always be modified. It looks like we have marked them all as MakeImmutable which serves as a warning that they should not be modified.
For constants I'm used to all upper case and underscores so the lowercase prefix is not helpful to me. I'm open to it if it actually has a specific meaning. When I first saw it I thought we were encoding type info in the symbolic name and if so I don't see a need for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r677675104
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByLexIntegrationTest.java
##########
@@ -0,0 +1,445 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.executor.sortedset;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_MIN_MAX_NOT_A_VALID_STRING;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.lang3.StringUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.netty.Coder;
+
+@RunWith(JUnitParamsRunner.class)
+public abstract class AbstractZRangeByLexIntegrationTest implements RedisIntegrationTest {
Review comment:
Per [the docs for ZRANGEBYLEX](https://redis.io/commands/zrangebylex): "If the elements in the sorted set have different scores, the returned elements are unspecified." so it's fine for us to differ from Redis here, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r677552662
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZRangeByLexIntegrationTest.java
##########
@@ -0,0 +1,445 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.executor.sortedset;
+
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_MIN_MAX_NOT_A_VALID_STRING;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.apache.geode.util.internal.UncheckedUtils.uncheckedCast;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.lang3.StringUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.netty.Coder;
+
+@RunWith(JUnitParamsRunner.class)
+public abstract class AbstractZRangeByLexIntegrationTest implements RedisIntegrationTest {
Review comment:
Although, not necessarily a correct dataset to run against, native redis will return all of these entries, but we don't:
```
@Test
public void validation() {
jedis.zadd(KEY, 1, "a1");
jedis.zadd(KEY, 2, "b1");
jedis.zadd(KEY, 1, "c1");
jedis.zadd(KEY, 1, "a2");
jedis.zadd(KEY, 2, "b2");
jedis.zadd(KEY, 1, "c2");
Set<String> result = jedis.zrangeByLex(KEY, "-", "+");
assertThat(result).containsExactly("a1", "b1", "c1", "a2", "b2", "c2");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675670823
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
@MakeImmutable
public static final byte[] bABSTTL = stringToBytes("ABSTTL");
- // AbstractZRangeExecutor
+ // Various ZRangeExecutors
@MakeImmutable
public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
@MakeImmutable
public static final byte[] bLIMIT = stringToBytes("LIMIT");
+ public static final byte bPLUS = SIMPLE_STRING_ID; // +
Review comment:
I can see the danger of having byte arrays that you want to be constants. In fact any array that you want to treat as a constant could be an issue because arrays can always be modified. Should all of these static final byte arrays be marked with the Immutable annotation? For constants I'm used to all upper case and underscores so the lowercase prefix is not helpful to me. I'm open to it if it actually has a specific meaning. When I first saw it I thought we were encoding type info in the symbolic name and if so I don't see a need for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r677029427
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -37,25 +39,32 @@
}
void parseLimitArguments(List<byte[]> commandElements, int commandIndex) {
- if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex - 2), bLIMIT)) {
+ String element = bytesToString(commandElements.get(commandIndex));
+ if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex), bLIMIT)) {
+ System.out.println("DONAL: throwing because " + element + " is not LIMIT");
Review comment:
Thanks for spotting that. It's gone now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675110924
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -636,4 +663,48 @@ public int getSizeInBytes() {
return 0;
}
}
+
+ // Dummy entry used to find the rank of an element with the given member name for lexically
+ // ordered sets
+ static class MemberDummyOrderedSetEntry extends AbstractOrderedSetEntry {
+ boolean isExclusive;
Review comment:
should these two boolean fields be final? It looks like they are never modfied
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/AbstractSortedSetRangeOptions.java
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.executor.sortedset;
+
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.equalsIgnoreCaseBytes;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bLIMIT;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bMINUS;
+
+import java.util.List;
+
+public abstract class AbstractSortedSetRangeOptions<T> {
+ boolean isMinExclusive;
+ T minimum;
+ boolean isMaxExclusive;
+ T maximum;
+ boolean hasLimit;
+ int offset;
+ int count;
+
+ AbstractSortedSetRangeOptions(byte[] minimumBytes, byte[] maximumBytes) {
+ parseMinimum(minimumBytes);
+ parseMaximum(maximumBytes);
+ }
+
+ void parseLimitArguments(List<byte[]> commandElements, int commandIndex) {
+ if (!equalsIgnoreCaseBytes(commandElements.get(commandIndex - 2), bLIMIT)) {
+ throw new IllegalArgumentException();
+ } else {
+ byte[] offsetBytes = commandElements.get(commandIndex - 1);
+ if (offsetBytes[0] == bMINUS) {
+ throw new NumberFormatException();
+ }
+ // Only set the values for the first limit we parse
+
+ int parsedOffset = narrowLongToInt(bytesToLong(offsetBytes));
+ int parsedCount = narrowLongToInt(bytesToLong(commandElements.get(commandIndex)));
+ if (!hasLimit) {
+ hasLimit = true;
+ this.offset = parsedOffset;
+ this.count = parsedCount;
+ if (this.count < 0) {
Review comment:
it seems a little better to only set "this.count" once. So do the < 0 check against parsedCount
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -443,6 +445,31 @@ long zrevrank(byte[] member) {
return result;
}
+ private List<byte[]> addLimitToRange(AbstractSortedSetRangeOptions<?> rangeOptions,
+ boolean withScores, int minIndex, int maxIndex) {
+ int count = Integer.MAX_VALUE;
+ if (rangeOptions.hasLimit()) {
+ count = rangeOptions.getCount();
+ minIndex += rangeOptions.getOffset();
+ if (minIndex > getSortedSetSize()) {
+ return Collections.emptyList();
+ }
+ }
+ Iterator<AbstractOrderedSetEntry> entryIterator =
+ scoreSet.getIndexRange(minIndex, Math.min(count, maxIndex - minIndex), false);
+
+ List<byte[]> result = new ArrayList<>();
Review comment:
Can we give this ArrayList an initialize size to make this code perform better? It seems like it would be Math.min(count, maxIndex - minIndex) and *2 if withScores.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
@MakeImmutable
public static final byte[] bABSTTL = stringToBytes("ABSTTL");
- // AbstractZRangeExecutor
+ // Various ZRangeExecutors
@MakeImmutable
public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
@MakeImmutable
public static final byte[] bLIMIT = stringToBytes("LIMIT");
+ public static final byte bPLUS = SIMPLE_STRING_ID; // +
Review comment:
what is the deal with the "b" prefix? At one point I thought it meant the constant was a byte array but here it is just a byte. When should the prefix be used? Also why does it exist? I have not seen this pattern before in java.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZRangeByScoreExecutor.java
##########
@@ -33,35 +30,35 @@
public class ZRangeByScoreExecutor extends AbstractExecutor {
@Override
public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
- RedisSortedSetCommands redisSortedSetCommands = context.getSortedSetCommands();
-
List<byte[]> commandElements = command.getProcessedCommand();
- SortedSetRangeOptions rangeOptions;
- boolean withScores = false;
+ SortedSetScoreRangeOptions rangeOptions;
try {
byte[] minBytes = commandElements.get(2);
byte[] maxBytes = commandElements.get(3);
- rangeOptions = new SortedSetRangeOptions(minBytes, maxBytes);
+ rangeOptions = new SortedSetScoreRangeOptions(minBytes, maxBytes);
} catch (NumberFormatException ex) {
return RedisResponse.error(ERROR_MIN_MAX_NOT_A_FLOAT);
}
// Native redis allows multiple "withscores" and "limit ? ?" clauses; the last "limit"
// clause overrides any previous ones
- if (commandElements.size() >= 5) {
- int currentCommandElement = 4;
+ // In order to match the error reporting behaviour of native Redis, the argument list must be
+ // parsed in reverse order. Stop parsing at index = 4, since 0 is the command name, 1 is the
+ // key, 2 is the min and 3 is the max
+ boolean withScores = false;
- while (currentCommandElement < commandElements.size()) {
+ if (commandElements.size() >= 5) {
Review comment:
This whole block of code is much the same as the one in ZRangeByLexExecutor. I think all you need is somehow to tell rangeOptions.parseLimitArguments that it should also parse the withScores flag. You could then ask rangeOptions if isWithScores is true.
You could have the method that does this whole for loop in rangeOptions in a method that returns a RedisResponse. If it returns one then this code would return that response. Otherwise it would continue
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -636,4 +663,48 @@ public int getSizeInBytes() {
return 0;
}
}
+
+ // Dummy entry used to find the rank of an element with the given member name for lexically
+ // ordered sets
+ static class MemberDummyOrderedSetEntry extends AbstractOrderedSetEntry {
+ boolean isExclusive;
+ boolean isMinimum;
+
+ MemberDummyOrderedSetEntry(byte[] member, double score, boolean isExclusive,
+ boolean isMinimum) {
+ this.member = member;
+ this.scoreBytes = null;
+ this.score = score;
+ this.isExclusive = isExclusive;
+ this.isMinimum = isMinimum;
+ }
+
+ @Override
+ public int compareMembers(byte[] array1, byte[] array2) {
+ int dummyNameComparison = checkDummyMemberNames(array1, array2);
+ if (dummyNameComparison != 0) {
+ return dummyNameComparison;
+ } else {
+ // If not using dummy member names, move on to actual lexical comparison
+ int stringComparison = javaImplementationOfAnsiCMemCmp(array1, array2);
+ if (stringComparison == 0) {
+ if (isMinimum && isExclusive || !isMinimum && !isExclusive) {
Review comment:
you could use the XOR operator to simplify this to "if (isMinimum ^ isExclusive)". Note that if they are both true or both false then XOR returns false so you need to flip your return value (if (A^B) return -1 else return 1).
I think I saw some other code recently in another PR of yours that could have used XOR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org