dschneider-pivotal commented on a change in pull request #6861:
URL: https://github.com/apache/geode/pull/6861#discussion_r707783555
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
+ for (OrderedSetEntry entry : set.members.values()) {
+ Double newScore;
+ if (aggregator.equals(ZAggregator.SUM)) {
+ newScore = recursivelySumScoresForMember(sets, entry.member, 0D);
+ } else if (aggregator.equals(ZAggregator.MAX)) {
+ newScore = recursivelyGetMaxScoreForMember(sets, entry.member,
Double.MIN_VALUE);
+ } else {
+ newScore = recursivelyGetMinScoreForMember(sets, entry.member,
Double.MAX_VALUE);
+ }
+
+ if (newScore != null) {
+ if (newScore.isNaN()) {
+ throw new ArithmeticException(ERROR_OPERATION_PRODUCED_NAN);
+ }
+ retVal.memberAdd(entry.getMember(), Coder.doubleToBytes(newScore));
+ }
+ }
+ }
+ return retVal;
+ }
+
+ private Double recursivelySumScoresForMember(List<RedisSortedSet> sets,
byte[] member,
+ Double runningTotal) {
+ if (sets.isEmpty()) {
+ return runningTotal;
+ }
+
+ if (runningTotal.isNaN()) {
+ return runningTotal;
+ }
+
+ if (sets.get(0).members.containsKey(member)) {
Review comment:
instead of doing containsKey followed by get, just do get and check for
it returning null. This does one map lookup instead of two
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
+ for (OrderedSetEntry entry : set.members.values()) {
Review comment:
Since all you care about seeing in this for loop is each member in
"members" I think you can just do this "for (byte[] member:
set.members.keySet())".
Or you could use "set.members.fastForEachKey(key->{...});"
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZInterStoreExecutor.java
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 class ZInterStoreExecutor 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<RedisKey> sourceKeys = new ArrayList<>((int) numKeys);
+ List<Double> weights = new ArrayList<>((int) numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+
+ while (argIterator.hasNext()) {
+ byte[] arg = argIterator.next();
+
+ if (sourceKeys.size() < numKeys) {
+ sourceKeys.add(new RedisKey(arg));
+ continue;
+ }
+
+ arg = toUpperCaseBytes(arg);
+ if (Arrays.equals(arg, bWEIGHTS)) {
+ if (!weights.isEmpty()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ try {
+ weights.add(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 (sourceKeys.size() != numKeys) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+
+ int bucket = command.getKey().getBucketId();
+ for (RedisKey key : sourceKeys) {
+ if (key.getBucketId() != bucket) {
+ return RedisResponse.crossSlot(ERROR_WRONG_SLOT);
+ }
+ }
+
+ List<ZKeyWeight> keyWeights = new ArrayList<>();
Review comment:
this list can be presized.
How about just having this List and get rid of the sourceKeys list and and
the weights list/array. Create the initial ZKeyWeight with just the key (its
double can default to 1). Then when you parse the weights you can call
setWeight on the correct ZKeyWeight.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -661,7 +793,7 @@ public static int javaImplementationOfAnsiCMemCmp(byte[]
array1, byte[] array2)
Sizeable {
byte[] member;
byte[] scoreBytes;
- double score;
+ Double score;
Review comment:
Why does this need to be an Object (Double)? The old "double" saves
memory.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
+ for (OrderedSetEntry entry : set.members.values()) {
+ Double newScore;
+ if (aggregator.equals(ZAggregator.SUM)) {
+ newScore = recursivelySumScoresForMember(sets, entry.member, 0D);
+ } else if (aggregator.equals(ZAggregator.MAX)) {
+ newScore = recursivelyGetMaxScoreForMember(sets, entry.member,
Double.MIN_VALUE);
+ } else {
+ newScore = recursivelyGetMinScoreForMember(sets, entry.member,
Double.MAX_VALUE);
+ }
+
+ if (newScore != null) {
+ if (newScore.isNaN()) {
+ throw new ArithmeticException(ERROR_OPERATION_PRODUCED_NAN);
+ }
+ retVal.memberAdd(entry.getMember(), Coder.doubleToBytes(newScore));
+ }
+ }
+ }
+ return retVal;
+ }
+
+ private Double recursivelySumScoresForMember(List<RedisSortedSet> sets,
byte[] member,
+ Double runningTotal) {
+ if (sets.isEmpty()) {
+ return runningTotal;
+ }
+
+ if (runningTotal.isNaN()) {
+ return runningTotal;
+ }
+
+ if (sets.get(0).members.containsKey(member)) {
+ Double score = sets.get(0).members.get(member).score;
Review comment:
use "double" here
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
+ for (OrderedSetEntry entry : set.members.values()) {
+ Double newScore;
+ if (aggregator.equals(ZAggregator.SUM)) {
+ newScore = recursivelySumScoresForMember(sets, entry.member, 0D);
+ } else if (aggregator.equals(ZAggregator.MAX)) {
+ newScore = recursivelyGetMaxScoreForMember(sets, entry.member,
Double.MIN_VALUE);
+ } else {
+ newScore = recursivelyGetMinScoreForMember(sets, entry.member,
Double.MAX_VALUE);
+ }
+
+ if (newScore != null) {
+ if (newScore.isNaN()) {
+ throw new ArithmeticException(ERROR_OPERATION_PRODUCED_NAN);
+ }
+ retVal.memberAdd(entry.getMember(), Coder.doubleToBytes(newScore));
+ }
+ }
+ }
+ return retVal;
+ }
+
+ private Double recursivelySumScoresForMember(List<RedisSortedSet> sets,
byte[] member,
Review comment:
I think the recursion in all three of these methods should be replaced
with a simple iteration of sets checking each one for member and computing the
sum/min/max. If it finds member in each set then it can store that value on the
retVal set.
Recursion is usually slower, harder for some to understand, and could have a
stack overflow.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZInterStoreExecutor.java
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 class ZInterStoreExecutor 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<RedisKey> sourceKeys = new ArrayList<>((int) numKeys);
+ List<Double> weights = new ArrayList<>((int) numKeys);
+ ZAggregator aggregator = ZAggregator.SUM;
+
+ while (argIterator.hasNext()) {
+ byte[] arg = argIterator.next();
+
+ if (sourceKeys.size() < numKeys) {
+ sourceKeys.add(new RedisKey(arg));
+ continue;
+ }
+
+ arg = toUpperCaseBytes(arg);
+ if (Arrays.equals(arg, bWEIGHTS)) {
+ if (!weights.isEmpty()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ for (int i = 0; i < numKeys; i++) {
+ if (!argIterator.hasNext()) {
+ return RedisResponse.error(ERROR_SYNTAX);
+ }
+ try {
+ weights.add(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()));
Review comment:
do you need a hasNext check here also?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -288,6 +289,52 @@ long zcount(SortedSetScoreRangeOptions rangeOptions) {
return byteIncr;
}
+ long zinterstore(RegionProvider regionProvider, RedisKey key,
List<ZKeyWeight> keyWeights,
+ ZAggregator aggregator) {
+ List<RedisSortedSet> sets = new ArrayList<>(keyWeights.size());
+ for (ZKeyWeight keyWeight : keyWeights) {
+ RedisSortedSet set =
+ regionProvider.getTypedRedisData(REDIS_SORTED_SET,
keyWeight.getKey(), false);
+
+ if (set == NULL_REDIS_SORTED_SET) {
+ continue;
+ }
+
+ double weight = keyWeight.getWeight();
+ RedisSortedSet weightedSet = new RedisSortedSet(Collections.emptyList());
+
+ for (AbstractOrderedSetEntry entry : set.members.values()) {
+ OrderedSetEntry existingValue = members.get(entry.member);
+ if (existingValue == null) {
+ double score;
+ // Redis math and Java math are different when handling infinity.
Specifically:
+ // Java: INFINITY * 0 = NaN
+ // Redis: INFINITY * 0 = 0
+ if (weight == 0) {
+ score = 0;
+ } else if (weight == 1) {
+ score = entry.getScore();
+ } else {
+ double newScore = entry.score * weight;
Review comment:
I think this code would be easier to understand if you just set the
existing "score" variable instead of introducing "newScore".
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
+ for (OrderedSetEntry entry : set.members.values()) {
+ Double newScore;
+ if (aggregator.equals(ZAggregator.SUM)) {
+ newScore = recursivelySumScoresForMember(sets, entry.member, 0D);
+ } else if (aggregator.equals(ZAggregator.MAX)) {
+ newScore = recursivelyGetMaxScoreForMember(sets, entry.member,
Double.MIN_VALUE);
+ } else {
+ newScore = recursivelyGetMinScoreForMember(sets, entry.member,
Double.MAX_VALUE);
+ }
+
+ if (newScore != null) {
+ if (newScore.isNaN()) {
+ throw new ArithmeticException(ERROR_OPERATION_PRODUCED_NAN);
+ }
+ retVal.memberAdd(entry.getMember(), Coder.doubleToBytes(newScore));
Review comment:
It seems like if you passed retVal in to recursively*ScoreForMember then
it could do the retVal.memberAdd if needed. That would also allow us to get rid
of this use of "Double". If you get rid of the recursion then these methods
could just take a member, input sets, and a set to fill.
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZInterStoreExecutor.java
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 class ZInterStoreExecutor 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<RedisKey> sourceKeys = new ArrayList<>((int) numKeys);
+ List<Double> weights = new ArrayList<>((int) numKeys);
Review comment:
could weights be made a "double[]"?
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -578,6 +625,91 @@ private int
getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
return result;
}
+ private RedisSortedSet getIntersection(List<RedisSortedSet> sets,
ZAggregator aggregator) {
+ RedisSortedSet retVal = new RedisSortedSet(Collections.emptyList());
+
+ for (RedisSortedSet set : sets) {
Review comment:
It looks to me like if you have 10 sets then for each member that is in
each set this will compute the score 10 times. I think we only need to compute
it once. So do we really need to iterate over all the sets at the top level?
Since we will only add members to the result that are in every one of the sets,
couldn't we just iterate over the members of one of the sets at the top level?
I think the best one to iterate would be the smallest one.
--
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]
