You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2025/03/19 21:14:48 UTC
[PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
dcapwell opened a new pull request, #3995:
URL: https://github.com/apache/cassandra/pull/3995
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004337363
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -363,7 +386,8 @@ protected TableMetadata defineTable(RandomSource rs, String ks)
.withCompression())
.withKeyspaceName(ks).withTableName("tbl")
.withSimpleColumnNames()
- .withDefaultTypeGen(supportedTypes())
+ .withDefaultTypeGen(supportedTypes(rs))
+ .withPrimaryColumnTypeGen(supportedPrimaryColumnTypes(rs))
Review Comment:
when primary keys can be collections there can be issues with having too many bytes and having our mutations be rejected. Keeping it primitive also improves readability for debug output...
Would be good to have a test that overrides this though, just to improve coverage...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004386347
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -493,6 +503,45 @@ private static void validateAnyOrder(ImmutableUniqueList<Symbol> columns, Set<Ro
if (actual.isEmpty()) sb.append("No rows returned");
else sb.append("Missing rows:\n").append(table(columns, missing));
}
+ if (!unexpected.isEmpty() && unexpected.size() == missing.size())
Review Comment:
this is extra human readable validation logic. `null` vs `<empty>` has been super hard to see from the above output, and the use case i was finding is that we had the correct rows returned, but 1 or more columns had a difference... so if this is the case we try to show which columns are off... this was super useful for detecting consistency issues with `null` and `<empty>`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004339713
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
.withoutTransaction()
.withoutTtl()
.withoutTimestamp()
- .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+ .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+ .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));
Review Comment:
each column will have a different distribution for when operators are allowed
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004465282
##########
test/unit/org/apache/cassandra/cql3/ast/ExpressionEvaluator.java:
##########
@@ -21,126 +21,152 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.ByteBuffer;
-import java.util.Optional;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
-import org.apache.cassandra.db.marshal.AbstractType;
+import javax.annotation.Nullable;
-import static java.util.Optional.of;
+import org.apache.cassandra.db.marshal.AbstractType;
public class ExpressionEvaluator
{
- public static Optional<Object> tryEval(Expression e)
+ @Nullable
+ public static Object eval(Expression e)
{
if (e instanceof Value)
- return of(((Value) e).value());
+ return ((Value) e).value();
if (e instanceof TypeHint)
- return tryEval(((TypeHint) e).e);
+ return eval(((TypeHint) e).e);
if (e instanceof Operator)
- return tryEval((Operator) e);
- return Optional.empty();
+ return eval((Operator) e);
+ throw new UnsupportedOperationException("Unexpected expression type " + e.getClass() + ": " + e.toCQL());
}
- public static Optional<Object> tryEval(Operator e)
+ @Nullable
+ public static Object eval(Operator e)
{
+ Object lhs = eval(e.left);
Review Comment:
the model `eval` method will put the "current" value as a `ByteBuffer` into the operation, this then means that the types might not match! to solve this we just convert w/e we see a BB.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2006118679
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -53,31 +53,30 @@ public AssignmentOperator(Kind kind, Expression right)
this.right = right;
}
- public static EnumSet<Kind> supportsOperators(AbstractType<?> type)
+ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTransaction)
{
type = type.unwrap();
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
+ if (type instanceof CollectionType && type.isMultiCell())
+ {
+ if (type instanceof SetType || type instanceof ListType)
+ return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof MapType)
+ {
+ // map supports subtract, but not map - map; only map - set!
Review Comment:
That's... an interesting finding. Should we have a Jira to add proper support?
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -893,28 +942,60 @@ private List<Clustering<ByteBuffer>> keys(Map<Symbol, List<? extends Expression>
return current.stream().map(BufferClustering::new).collect(Collectors.toList());
}
+ private static ByteBuffer eval(Symbol col, @Nullable ByteBuffer current, Expression e)
+ {
+ if (!(e instanceof AssignmentOperator)) return eval(e);
+ // multi cell collections have the property that they do update even if the current value is null
+ boolean isFancy = col.type().isCollection() && col.type().isMultiCell();
Review Comment:
Curious about the `isFancy` name choosing :-)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2008142902
##########
test/unit/org/apache/cassandra/utils/Generators.java:
##########
@@ -492,13 +494,20 @@ public static <T> Gen<Set<T>> set(Gen<T> gen, Gen<Integer> sizeGen)
};
}
- public static <T extends Comparable<? super T>> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
+ public static <T> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
{
- return set(gen, sizeGen).map(t -> {
- List<T> list = new ArrayList<>(t);
- list.sort(Comparator.naturalOrder());
- return list;
- });
+ return rnd -> {
+ int size = sizeGen.generate(rnd);
+ Set<T> set = Sets.newHashSetWithExpectedSize(size);
+ List<T> output = new ArrayList<>(size);
+ for (int i = 0; i < size; i++)
+ {
+ T value;
+ while (!set.add(value = gen.generate(rnd))) {}
Review Comment:
for now no. There are better impl in `Gens` and when i migrate off of quick theories we can just use the better versions; so no reason to handle here...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2006415170
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -893,28 +942,60 @@ private List<Clustering<ByteBuffer>> keys(Map<Symbol, List<? extends Expression>
return current.stream().map(BufferClustering::new).collect(Collectors.toList());
}
+ private static ByteBuffer eval(Symbol col, @Nullable ByteBuffer current, Expression e)
+ {
+ if (!(e instanceof AssignmentOperator)) return eval(e);
+ // multi cell collections have the property that they do update even if the current value is null
+ boolean isFancy = col.type().isCollection() && col.type().isMultiCell();
Review Comment:
i felt it was fancy!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004333889
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -88,10 +101,20 @@ protected void preCheck(Cluster cluster, Property.StatefulBuilder builder)
// CQL_DEBUG_APPLY_OPERATOR = true;
}
- protected TypeGenBuilder supportedTypes()
+ protected TypeGenBuilder supportedTypes(RandomSource rs)
{
- return AbstractTypeGenerators.withoutUnsafeEquality(AbstractTypeGenerators.builder()
- .withTypeKinds(AbstractTypeGenerators.TypeKind.PRIMITIVE));
+ return AbstractTypeGenerators.builder()
+ .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+ .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))
Review Comment:
before we used uniform random so each test had the same weights. Now we select a new distribution for each test run (some tests will only be primitives, others will be mostly maps, etc.)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004469337
##########
test/unit/org/apache/cassandra/utils/Generators.java:
##########
@@ -492,13 +494,20 @@ public static <T> Gen<Set<T>> set(Gen<T> gen, Gen<Integer> sizeGen)
};
}
- public static <T extends Comparable<? super T>> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
+ public static <T> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
{
- return set(gen, sizeGen).map(t -> {
- List<T> list = new ArrayList<>(t);
- list.sort(Comparator.naturalOrder());
- return list;
- });
+ return rnd -> {
+ int size = sizeGen.generate(rnd);
+ Set<T> set = Sets.newHashSetWithExpectedSize(size);
+ List<T> output = new ArrayList<>(size);
+ for (int i = 0; i < size; i++)
+ {
+ T value;
+ while (!set.add(value = gen.generate(rnd))) {}
Review Comment:
this isn't as safe as `uniqueBestEffort` from accord... once accord merges we can talk about unifying all this stuff...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004334846
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -88,10 +101,20 @@ protected void preCheck(Cluster cluster, Property.StatefulBuilder builder)
// CQL_DEBUG_APPLY_OPERATOR = true;
}
- protected TypeGenBuilder supportedTypes()
+ protected TypeGenBuilder supportedTypes(RandomSource rs)
{
- return AbstractTypeGenerators.withoutUnsafeEquality(AbstractTypeGenerators.builder()
- .withTypeKinds(AbstractTypeGenerators.TypeKind.PRIMITIVE));
+ return AbstractTypeGenerators.builder()
+ .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+ .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))
+ .withUserTypeFields(AbstractTypeGenerators.UserTypeFieldsGen.simpleNames())
+ .withMaxDepth(1);
Review Comment:
ATM we are not testing nested access (will need to eventually) so lowered as the cql literal output was so large it got hard to read (UDT of map of UDT of map of UDT....)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004466704
##########
test/unit/org/apache/cassandra/utils/ASTGenerators.java:
##########
@@ -770,12 +784,12 @@ private void generateRemaining(RandomnessSource rnd,
}
}
}
- if (kind == Mutation.Kind.UPDATE && isTransaction)
Review Comment:
collections can do things outside of transactions, so need to push this into `supportsOperators`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell closed pull request #3995: CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types
URL: https://github.com/apache/cassandra/pull/3995
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004467684
##########
test/unit/org/apache/cassandra/utils/AbstractTypeGenerators.java:
##########
@@ -289,19 +298,27 @@ public TypeGenBuilder()
public TypeGenBuilder(TypeGenBuilder other)
{
maxDepth = other.maxDepth;
- kinds = other.kinds == null ? null : EnumSet.copyOf(other.kinds);
+ kinds = other.kinds;
typeKindGen = other.typeKindGen;
defaultSizeGen = other.defaultSizeGen;
vectorSizeGen = other.vectorSizeGen;
tupleSizeGen = other.tupleSizeGen;
- udtName = other.udtName;
udtSizeGen = other.udtSizeGen;
+ compositeSizeGen = other.compositeSizeGen;
primitiveGen = other.primitiveGen;
+ compositeElementGen = other.compositeElementGen;
userTypeKeyspaceGen = other.userTypeKeyspaceGen;
defaultSetKeyFunc = other.defaultSetKeyFunc;
- compositeElementGen = other.compositeElementGen;
- compositeSizeGen = other.compositeSizeGen;
typeFilter = other.typeFilter;
+ udtName = other.udtName;
+ multiCellGen = other.multiCellGen;
Review Comment:
this was missing so i took the fields in object order and recreated this constructor just in case there was more; looks like it was just this one 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004332743
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -88,10 +101,20 @@ protected void preCheck(Cluster cluster, Property.StatefulBuilder builder)
// CQL_DEBUG_APPLY_OPERATOR = true;
}
- protected TypeGenBuilder supportedTypes()
+ protected TypeGenBuilder supportedTypes(RandomSource rs)
{
- return AbstractTypeGenerators.withoutUnsafeEquality(AbstractTypeGenerators.builder()
- .withTypeKinds(AbstractTypeGenerators.TypeKind.PRIMITIVE));
+ return AbstractTypeGenerators.builder()
+ .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+ .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))
+ .withUserTypeFields(AbstractTypeGenerators.UserTypeFieldsGen.simpleNames())
Review Comment:
not testing UDT serialization so simple names makes it so much easier to read...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2008163806
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
.withoutTransaction()
.withoutTtl()
.withoutTimestamp()
- .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+ .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+ .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));
Review Comment:
We need different assignment operators to repro (`+= v` vs. `= {v}`) but not necessarily different distributions of operators, which is what's introduced here - is that right?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004397629
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -731,7 +780,7 @@ private static boolean matches(ByteBuffer value, List<? extends Expression> cond
for (Expression e : conditions)
{
ByteBuffer expected = eval(e);
- if (expected.equals(value))
+ if (expected != null && expected.equals(value))
Review Comment:
`eval` is now `Nullable` but this code path shouldn't really ever see a null... maybe if you did something like `[1] - [1]` then we could get a `null` here... mostly added to be correct and defensive (and intellij yelled at me)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004386755
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -493,6 +503,45 @@ private static void validateAnyOrder(ImmutableUniqueList<Symbol> columns, Set<Ro
if (actual.isEmpty()) sb.append("No rows returned");
else sb.append("Missing rows:\n").append(table(columns, missing));
}
+ if (!unexpected.isEmpty() && unexpected.size() == missing.size())
+ {
+ // good chance a column differs
+ StringBuilder finalSb = sb;
+ Runnable runOnce = new Runnable()
Review Comment:
i think Alex created a reusable function for this in accord... would need to find it
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2006603166
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -523,6 +561,8 @@ private List<Symbol> multiColumnQueryColumns()
List<Symbol> allowedColumns = searchableColumns;
if (hasMultiNodeMultiColumnAllowFilteringWithLocalWritesIssue())
allowedColumns = nonPkIndexedColumns;
+ if (IGNORED_ISSUES.contains(KnownIssue.SAI_AND_VECTOR_COLUMNS) && !indexes.isEmpty())
Review Comment:
added to get clean CI. Something is up with multi column queries that touch both SAI and `vector` type. It doesn't look like the `vector` must be indexed to get incorrect results, so doesn't feel like we are selecting a bad index here
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004463744
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -141,7 +141,7 @@ else if (((isFrozen(type) && !type.isVector()) || StorageAttachedIndex.SUPPORTED
public EnumSet<QueryType> supportedQueries(AbstractType<?> type)
{
type = type.unwrap();
- if (IndexTermType.isEqOnlyType(type))
+ if (IndexTermType.isEqOnlyType(type) || type.isCollection() || type.isUDT() || type.isTuple())
Review Comment:
@maedhroz should we push this into `IndexTermType.isEqOnlyType`? Its also not 100% true as they do support eq for the sub-elements, just not the field as a whole
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -141,7 +141,7 @@ else if (((isFrozen(type) && !type.isVector()) || StorageAttachedIndex.SUPPORTED
public EnumSet<QueryType> supportedQueries(AbstractType<?> type)
{
type = type.unwrap();
- if (IndexTermType.isEqOnlyType(type))
+ if (IndexTermType.isEqOnlyType(type) || type.isCollection() || type.isUDT() || type.isTuple())
Review Comment:
@maedhroz should we push this into `IndexTermType.isEqOnlyType`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004469337
##########
test/unit/org/apache/cassandra/utils/Generators.java:
##########
@@ -492,13 +494,20 @@ public static <T> Gen<Set<T>> set(Gen<T> gen, Gen<Integer> sizeGen)
};
}
- public static <T extends Comparable<? super T>> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
+ public static <T> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
{
- return set(gen, sizeGen).map(t -> {
- List<T> list = new ArrayList<>(t);
- list.sort(Comparator.naturalOrder());
- return list;
- });
+ return rnd -> {
+ int size = sizeGen.generate(rnd);
+ Set<T> set = Sets.newHashSetWithExpectedSize(size);
+ List<T> output = new ArrayList<>(size);
+ for (int i = 0; i < size; i++)
+ {
+ T value;
+ while (!set.add(value = gen.generate(rnd))) {}
Review Comment:
this isn't as safe as `uniqueBestEffort` from accord (if gen is a boolean gen and size is 3, this will loop forever)... once accord merges we can talk about unifying all this stuff...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#issuecomment-2744215498
merged
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004465654
##########
test/unit/org/apache/cassandra/cql3/ast/ExpressionEvaluator.java:
##########
@@ -21,126 +21,152 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.ByteBuffer;
-import java.util.Optional;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
-import org.apache.cassandra.db.marshal.AbstractType;
+import javax.annotation.Nullable;
-import static java.util.Optional.of;
+import org.apache.cassandra.db.marshal.AbstractType;
public class ExpressionEvaluator
{
- public static Optional<Object> tryEval(Expression e)
+ @Nullable
+ public static Object eval(Expression e)
{
if (e instanceof Value)
- return of(((Value) e).value());
+ return ((Value) e).value();
if (e instanceof TypeHint)
- return tryEval(((TypeHint) e).e);
+ return eval(((TypeHint) e).e);
if (e instanceof Operator)
- return tryEval((Operator) e);
- return Optional.empty();
+ return eval((Operator) e);
+ throw new UnsupportedOperationException("Unexpected expression type " + e.getClass() + ": " + e.toCQL());
}
- public static Optional<Object> tryEval(Operator e)
+ @Nullable
+ public static Object eval(Operator e)
{
+ Object lhs = eval(e.left);
+ if (lhs instanceof ByteBuffer)
+ lhs = e.left.type().compose((ByteBuffer) lhs);
+ Object rhs = eval(e.right);
+ if (rhs instanceof ByteBuffer)
+ rhs = e.right.type().compose((ByteBuffer) rhs);
switch (e.kind)
{
case ADD:
{
- var lhsOpt = tryEval(e.left);
- var rhsOpt = tryEval(e.right);
- if (lhsOpt.isEmpty() || rhsOpt.isEmpty())
- return Optional.empty();
- Object lhs = lhsOpt.get();
- Object rhs = rhsOpt.get();
if (lhs instanceof Byte)
- return of((byte) (((Byte) lhs) + ((Byte) rhs)));
+ return (byte) (((Byte) lhs) + ((Byte) rhs));
if (lhs instanceof Short)
- return of((short) (((Short) lhs) + ((Short) rhs)));
+ return (short) (((Short) lhs) + ((Short) rhs));
if (lhs instanceof Integer)
- return of((int) (((Integer) lhs) + ((Integer) rhs)));
+ return (int) (((Integer) lhs) + ((Integer) rhs));
if (lhs instanceof Long)
- return of((long) (((Long) lhs) + ((Long) rhs)));
+ return (long) (((Long) lhs) + ((Long) rhs));
if (lhs instanceof Float)
- return of((float) (((Float) lhs) + ((Float) rhs)));
+ return (float) (((Float) lhs) + ((Float) rhs));
if (lhs instanceof Double)
- return of((double) (((Double) lhs) + ((Double) rhs)));
+ return (double) (((Double) lhs) + ((Double) rhs));
if (lhs instanceof BigInteger)
- return of(((BigInteger) lhs).add((BigInteger) rhs));
+ return ((BigInteger) lhs).add((BigInteger) rhs);
if (lhs instanceof BigDecimal)
- return of(((BigDecimal) lhs).add((BigDecimal) rhs));
+ return ((BigDecimal) lhs).add((BigDecimal) rhs);
if (lhs instanceof String)
- return of(lhs.toString() + rhs.toString());
+ return lhs.toString() + rhs.toString();
+ if (lhs instanceof Set)
+ {
+ Set<Object> accum = new HashSet<>((Set<Object>) lhs);
+ accum.addAll((Set<Object>) rhs);
+ return accum;
+ }
+ if (lhs instanceof List)
+ {
+ List<Object> accum = new ArrayList<>((List<Object>) lhs);
+ accum.addAll((List<Object>) rhs);
+ return accum;
+ }
+ if (lhs instanceof Map)
+ {
+ Map<Object, Object> accum = new HashMap<>((Map<Object, Object>) lhs);
+ accum.putAll((Map<Object, Object>) rhs);
+ return accum;
+ }
throw new UnsupportedOperationException("Unexpected type: " + lhs.getClass());
}
case SUBTRACT:
{
- var lhsOpt = tryEval(e.left);
- var rhsOpt = tryEval(e.right);
- if (lhsOpt.isEmpty() || rhsOpt.isEmpty())
- return Optional.empty();
- Object lhs = lhsOpt.get();
- Object rhs = rhsOpt.get();
if (lhs instanceof Byte)
- return of((byte) (((Byte) lhs) - ((Byte) rhs)));
+ return (byte) (((Byte) lhs) - ((Byte) rhs));
if (lhs instanceof Short)
- return of((short) (((Short) lhs) - ((Short) rhs)));
+ return (short) (((Short) lhs) - ((Short) rhs));
if (lhs instanceof Integer)
- return of((int) (((Integer) lhs) - ((Integer) rhs)));
+ return (int) (((Integer) lhs) - ((Integer) rhs));
if (lhs instanceof Long)
- return of((long) (((Long) lhs) - ((Long) rhs)));
+ return (long) (((Long) lhs) - ((Long) rhs));
if (lhs instanceof Float)
- return of((float) (((Float) lhs) - ((Float) rhs)));
+ return (float) (((Float) lhs) - ((Float) rhs));
if (lhs instanceof Double)
- return of((double) (((Double) lhs) - ((Double) rhs)));
+ return (double) (((Double) lhs) - ((Double) rhs));
if (lhs instanceof BigInteger)
- return of(((BigInteger) lhs).subtract((BigInteger) rhs));
+ return ((BigInteger) lhs).subtract((BigInteger) rhs);
if (lhs instanceof BigDecimal)
- return of(((BigDecimal) lhs).subtract((BigDecimal) rhs));
+ return ((BigDecimal) lhs).subtract((BigDecimal) rhs);
+ if (lhs instanceof Set)
+ {
+ Set<Object> accum = new HashSet<>((Set<Object>) lhs);
+ accum.removeAll((Set<Object>) rhs);
+ return accum.isEmpty() ? null : accum;
+ }
+ if (lhs instanceof List)
+ {
+ List<Object> accum = new ArrayList<>((List<Object>) lhs);
+ accum.removeAll((List<Object>) rhs);
+ return accum.isEmpty() ? null : accum;
+ }
+ if (lhs instanceof Map)
Review Comment:
this code path is currently untested, only part of this patch that wasn't validated
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004462017
##########
test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java:
##########
@@ -135,7 +141,13 @@ private long[] toDescriptor(ImmutableUniqueList<Symbol> positions, Map<Symbol, B
Symbol column = positions.get(i);
if (values.containsKey(column))
{
- long vd = factory.valueCache.deflate(new Value(column.type(), values.get(column)));
+ ByteBuffer value = values.get(column);
+ if (value == null || !value.hasRemaining() && (column.type().isUDT() && column.type().isMultiCell()))
Review Comment:
user type is the only multi cell type i see that allows `<empty>` so this check should be fine; can expand if we find more cases
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2007674972
##########
test/unit/org/apache/cassandra/utils/Generators.java:
##########
@@ -492,13 +494,20 @@ public static <T> Gen<Set<T>> set(Gen<T> gen, Gen<Integer> sizeGen)
};
}
- public static <T extends Comparable<? super T>> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
+ public static <T> Gen<List<T>> uniqueList(Gen<T> gen, Gen<Integer> sizeGen)
{
- return set(gen, sizeGen).map(t -> {
- List<T> list = new ArrayList<>(t);
- list.sort(Comparator.naturalOrder());
- return list;
- });
+ return rnd -> {
+ int size = sizeGen.generate(rnd);
+ Set<T> set = Sets.newHashSetWithExpectedSize(size);
+ List<T> output = new ArrayList<>(size);
+ for (int i = 0; i < size; i++)
+ {
+ T value;
+ while (!set.add(value = gen.generate(rnd))) {}
Review Comment:
In the meantime, want to set an iteration limit with a specific error? Just to make it a bit easier to figure out what's happening if a test is timing out.
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -78,6 +79,17 @@
public class SingleNodeTableWalkTest extends StatefulASTBase
{
+ private static final Gen<Gen<Boolean>> BOOLEAN_DISTRIBUTION = Gens.bools().mixedDistribution();
+ //TODO (coverage): COMPOSITE, DYNAMIC_COMPOSITE
+ private static Gen<Gen<TypeKind>> TYPE_KIND_DISTRIBUTION = Gens.mixedDistribution(TypeKind.PRIMITIVE,
+ TypeKind.SET, TypeKind.LIST, TypeKind.MAP,
+ TypeKind.TUPLE, TypeKind.UDT,
+ TypeKind.VECTOR
+ );
+ private static Gen<Gen<AbstractType<?>>> PRIMITIVE_DISTRIBUTION = Gens.mixedDistribution(AbstractTypeGenerators.knownPrimitiveTypes()
+ .stream()
+ .filter(t -> !AbstractTypeGenerators.isUnsafeEquality(t))
+ .collect(Collectors.toList()));
Review Comment:
Can be final
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
.withoutTransaction()
.withoutTtl()
.withoutTimestamp()
- .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+ .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+ .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));
Review Comment:
Required for CASSANDRA-20449 repro at least 😄
##########
test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java:
##########
@@ -135,7 +141,13 @@ private long[] toDescriptor(ImmutableUniqueList<Symbol> positions, Map<Symbol, B
Symbol column = positions.get(i);
if (values.containsKey(column))
{
- long vd = factory.valueCache.deflate(new Value(column.type(), values.get(column)));
+ ByteBuffer value = values.get(column);
+ if (value == null || !value.hasRemaining() && (column.type().isUDT() && column.type().isMultiCell()))
Review Comment:
Can you add this as a comment inline?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004335554
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -206,7 +229,7 @@ protected List<CreateIndexDDL.Indexer> supportedIndexers()
builder.value(pk, key.bufferAt(pks.indexOf(pk)));
- List<Symbol> searchableColumns = state.nonPartitionColumns;
+ List<Symbol> searchableColumns = state.searchableNonPartitionColumns;
Review Comment:
multi cell collections/udt can not be searched, so just using the non-partition columns wasn't enough
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2004464762
##########
test/unit/org/apache/cassandra/cql3/ast/ExpressionEvaluator.java:
##########
@@ -21,126 +21,152 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.ByteBuffer;
-import java.util.Optional;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
-import org.apache.cassandra.db.marshal.AbstractType;
+import javax.annotation.Nullable;
-import static java.util.Optional.of;
+import org.apache.cassandra.db.marshal.AbstractType;
public class ExpressionEvaluator
{
- public static Optional<Object> tryEval(Expression e)
+ @Nullable
+ public static Object eval(Expression e)
Review Comment:
returning `Optional` was just causing confusion, and all code paths did `.get()` anyways.
I also needed the ability to return `null` rather than `empty` which i couldn't do with `Optional`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2006414822
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -53,31 +53,30 @@ public AssignmentOperator(Kind kind, Expression right)
this.right = right;
}
- public static EnumSet<Kind> supportsOperators(AbstractType<?> type)
+ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTransaction)
{
type = type.unwrap();
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
+ if (type instanceof CollectionType && type.isMultiCell())
+ {
+ if (type instanceof SetType || type instanceof ListType)
+ return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof MapType)
+ {
+ // map supports subtract, but not map - map; only map - set!
Review Comment:
i wouldn't add a JIRA unless someone wants that feature. `map -=` acts on the key level, which can be done without a transaction (CAS or Accord), where as `remove(key, value)` would need to know the current value, which isn't behavior we like to support outside of transactions.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3995:
URL: https://github.com/apache/cassandra/pull/3995#discussion_r2008140165
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
.withoutTransaction()
.withoutTtl()
.withoutTimestamp()
- .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+ .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+ .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));
Review Comment:
wasn't required; we hit the issue before this change. This change is to better cover different edge cases
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org