DonalEvans commented on a change in pull request #6861:
URL: https://github.com/apache/geode/pull/6861#discussion_r717812025
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -607,6 +669,87 @@ private void addIfMatching(GlobPattern matchPattern,
List<byte[]> resultList, by
}
}
+ private void computeIntersection(List<RedisSortedSet> sets, ZAggregator
aggregator) {
Review comment:
This method and the ones below it are no longer used and can be removed.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -676,8 +819,8 @@ public static int javaImplementationOfAnsiCMemCmp(byte[]
array1, byte[] array2)
public abstract static class AbstractOrderedSetEntry
implements Comparable<AbstractOrderedSetEntry>,
Sizeable {
- byte[] member;
- double score;
+ protected byte[] member;
+ protected double score = 0D;
Review comment:
The visibility of these fields can be put back to being packing-private,
as they don't need to be accessed by anything outside of this package. There's
even an argument for making them private and having them only be accessed via
getters and set via package-private setters.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -84,6 +85,10 @@ public int getSizeInBytes() {
}
}
+ RedisSortedSet(int size) {
Review comment:
This is currently only used in the
`RedisSortedSetCommandsFunctionExecutor.zunionstore()` method. Other places
that could use this constructor to create an empty `RedisSortedSet` without
needing to allocate a `double[]` are
`RedisSortedSetCommandsFunctionExecutor.zinterstore()` and the
`NullRedisSortedSet` constructor.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSetCommandsFunctionExecutor.java
##########
@@ -164,7 +174,7 @@ public long zrevrank(RedisKey key, byte[] member) {
@Override
public long zunionstore(RedisKey destinationKey, List<ZKeyWeight> keyWeights,
ZAggregator aggregator) {
- List<RedisKey> keysToLock = new ArrayList<>(keyWeights.size());
+ List<RedisKey> keysToLock = getKeysToLock(destinationKey, keyWeights);
Review comment:
Since we're calling `getKeysToLock()` here, we don't need to iterate
over the `ZKeyWeight` list and repopulate the `keysToLock` list below.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZStoreExecutor.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.RedisConstants.ERROR_KEY_REQUIRED;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.Coder.toUpperCaseBytes;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bAGGREGATE;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bWEIGHTS;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.ListIterator;
+
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public abstract class ZStoreExecutor extends AbstractExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ ListIterator<byte[]> argIterator = commandElements.listIterator();
+ RedisResponse syntaxErrorResponse = RedisResponse.error(ERROR_SYNTAX);
+
+ // Skip command and destination key
+ argIterator.next();
+ argIterator.next();
+
+ // get the number of keys
+ int numKeys;
+ try {
+ numKeys = narrowLongToInt(Coder.bytesToLong(argIterator.next()));
+ if (numKeys > commandElements.size() - 2) {
+ return syntaxErrorResponse;
+ } else if (numKeys <= 0) {
+ return RedisResponse.error(ERROR_KEY_REQUIRED);
+ }
+ } catch (NumberFormatException ex) {
+ return syntaxErrorResponse;
+ }
+
+ List<ZKeyWeight> keyWeights = new ArrayList<>(numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+ byte[] argument;
+
+ // get all the keys
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)
+ || Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ return syntaxErrorResponse;
+ }
+ keyWeights.add(new ZKeyWeight(new RedisKey(argument), 1D));
+ }
+
+ while (argIterator.hasNext()) {
+ argument = argIterator.next();
+ // found AGGREGATE keyword; parse aggregate
+ if (Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)) {
+ return syntaxErrorResponse; // there must be an aggregate between
'AGGREGATE' & 'WEIGHTS'
+ }
+ try {
+ aggregator =
ZAggregator.valueOf(Coder.bytesToString(toUpperCaseBytes(argument)));
+ } catch (IllegalArgumentException e) {
+ return syntaxErrorResponse;
+ }
+ // found WEIGHTS keyword; parse weights
+ } else if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)) {
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ return syntaxErrorResponse; // there must be # weights between
'WEIGHTS' & 'AGGREGATE'
+ }
Review comment:
This behaviour doesn't match native Redis. The below test fails with
Radish but passes with native Redis:
```
@Test
public void
shouldReturnWeightNotAValidFloat_givenWeightsFollowedByCorrectNumberOfArgumentsIncludingAggregate()
{
assertThatThrownBy(
() -> jedis.sendCommand(NEW_SET, Protocol.Command.ZINTERSTORE,
NEW_SET, "3",
KEY1, KEY2, KEY3, "WEIGHTS", "1", "AGGREGATE", "SUM"))
.hasMessage("ERR " +
RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT);
}
```
I think that this check is not needed, because we only care if there are
enough weights specified (if there aren't, `argIterator.hasNext()` returns
false and we return a syntax error) and that those weights are valid floats (if
they're not, we get a `NumberFormatException` below and return
`ERROR_WEIGHT_NOT_A_FLOAT`).
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZStoreExecutor.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.RedisConstants.ERROR_KEY_REQUIRED;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.Coder.toUpperCaseBytes;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bAGGREGATE;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bWEIGHTS;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.ListIterator;
+
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public abstract class ZStoreExecutor extends AbstractExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ ListIterator<byte[]> argIterator = commandElements.listIterator();
+ RedisResponse syntaxErrorResponse = RedisResponse.error(ERROR_SYNTAX);
+
+ // Skip command and destination key
+ argIterator.next();
+ argIterator.next();
+
+ // get the number of keys
+ int numKeys;
+ try {
+ numKeys = narrowLongToInt(Coder.bytesToLong(argIterator.next()));
+ if (numKeys > commandElements.size() - 2) {
+ return syntaxErrorResponse;
+ } else if (numKeys <= 0) {
+ return RedisResponse.error(ERROR_KEY_REQUIRED);
+ }
+ } catch (NumberFormatException ex) {
+ return syntaxErrorResponse;
+ }
+
+ List<ZKeyWeight> keyWeights = new ArrayList<>(numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+ byte[] argument;
+
+ // get all the keys
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)
+ || Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ return syntaxErrorResponse;
+ }
Review comment:
This behaviour is incorrect. "WEIGHTS" and "AGGREGATE" are both valid
Redis key names. The below test fails with our implementation, but passes on
native Redis:
```
@Test
public void shouldNotReturnError_givenKeysNamedWeightsOrAggregate() {
String weights = "WEIGHTS";
jedis.zadd(weights, 1, "member");
String aggregate = "AGGREGATE";
jedis.zadd(aggregate, 1, "member");
jedis.zinterstore(weights, weights, weights);
jedis.zinterstore(aggregate, aggregate, aggregate);
}
```
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZStoreExecutor.java
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.RedisConstants.ERROR_SYNTAX;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
+import static org.apache.geode.redis.internal.netty.Coder.toUpperCaseBytes;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bAGGREGATE;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bWEIGHTS;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public abstract class ZStoreExecutor extends AbstractExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+
+ Iterator<byte[]> argIterator = commandElements.iterator();
+ // Skip command and destination key
+ argIterator.next();
+ argIterator.next();
+
+ long numKeys;
+ try {
+ numKeys = Coder.bytesToLong(argIterator.next());
+ } catch (NumberFormatException ex) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+
+ // Rough validation so that we can use numKeys to initialize the array
sizes below.
+ if (numKeys > commandElements.size()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+
+ List<ZKeyWeight> keyWeights = new ArrayList<>((int) numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+
+ while (argIterator.hasNext()) {
+ byte[] arg = argIterator.next();
+
+ if (keyWeights.size() < numKeys) {
+ keyWeights.add(new ZKeyWeight(new RedisKey(arg), 1D));
+ continue;
+ }
+
+ arg = toUpperCaseBytes(arg);
+ if (Arrays.equals(arg, bWEIGHTS)) {
+ if (!allWeightsAreOne(keyWeights)) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ try {
+
keyWeights.get(i).setWeight(Coder.bytesToDouble(argIterator.next()));
+ } catch (NumberFormatException nex) {
+ return RedisResponse.error(ERROR_WEIGHT_NOT_A_FLOAT);
+ }
+ }
+ continue;
+ }
+
+ if (Arrays.equals(arg, bAGGREGATE)) {
+ try {
+ aggregator =
ZAggregator.valueOf(Coder.bytesToString(argIterator.next()));
+ } catch (IllegalArgumentException | NoSuchElementException e) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ continue;
+ }
+
+ // End up here if we have more keys than weights
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+
+ if (keyWeights.size() != numKeys) {
Review comment:
With the current state of the code, when I comment out this check,
`AbstractZInterStoreIntegrationTest.shouldError_givenNumKeysTooLarge()` passes.
If we specify too many keys, we catch that on line 54 and return there.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZStoreExecutor.java
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.RedisConstants.ERROR_KEY_REQUIRED;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_WEIGHT_NOT_A_FLOAT;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_SLOT;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.Coder.toUpperCaseBytes;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bAGGREGATE;
+import static
org.apache.geode.redis.internal.netty.StringBytesGlossary.bWEIGHTS;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.ListIterator;
+
+import org.apache.geode.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.executor.AbstractExecutor;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public abstract class ZStoreExecutor extends AbstractExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ ListIterator<byte[]> argIterator = commandElements.listIterator();
+ RedisResponse syntaxErrorResponse = RedisResponse.error(ERROR_SYNTAX);
+
+ // Skip command and destination key
+ argIterator.next();
+ argIterator.next();
+
+ // get the number of keys
+ int numKeys;
+ try {
+ numKeys = narrowLongToInt(Coder.bytesToLong(argIterator.next()));
+ if (numKeys > commandElements.size() - 2) {
+ return syntaxErrorResponse;
+ } else if (numKeys <= 0) {
+ return RedisResponse.error(ERROR_KEY_REQUIRED);
+ }
+ } catch (NumberFormatException ex) {
+ return syntaxErrorResponse;
+ }
+
+ List<ZKeyWeight> keyWeights = new ArrayList<>(numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+ byte[] argument;
+
+ // get all the keys
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)
+ || Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ return syntaxErrorResponse;
+ }
+ keyWeights.add(new ZKeyWeight(new RedisKey(argument), 1D));
+ }
+
+ while (argIterator.hasNext()) {
+ argument = argIterator.next();
+ // found AGGREGATE keyword; parse aggregate
+ if (Arrays.equals(toUpperCaseBytes(argument), bAGGREGATE)) {
+ if (!argIterator.hasNext()) {
+ return syntaxErrorResponse;
+ }
+ argument = argIterator.next();
+ if (Arrays.equals(toUpperCaseBytes(argument), bWEIGHTS)) {
+ return syntaxErrorResponse; // there must be an aggregate between
'AGGREGATE' & 'WEIGHTS'
+ }
Review comment:
This check is unnecessary, as the below one, where we try to set the
`ZAggregator`, will return the same response if the next argument is not a
valid aggregator type.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
