yifan-c commented on code in PR #3562:
URL: https://github.com/apache/cassandra/pull/3562#discussion_r1931516221
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraint.java:
##########
@@ -42,10 +69,17 @@ public interface ColumnConstraint
void evaluate(Class<? extends AbstractType> valueType, Object columnValue)
throws ConstraintViolationException;
Review Comment:
This can be changed to
```suggestion
void evaluate(AbstractType<?> valueType, ByteBuffer columnValue) throws
ConstraintViolationException;
```
##########
src/java/org/apache/cassandra/cql3/constraints/ConstraintFunction.java:
##########
@@ -34,19 +33,14 @@ public interface ConstraintFunction
String getName();
/**
- * Method that provides the execution of the condition. It can either
succeed or throw a {@link ConstraintViolationException}.
- *
- * @param valueType
- * @param relationType
- * @param term
- * @param columnValue
+ * Method that performs the actual condition test, executed during the
write path.
+ * It the test is not successful, it throws a {@link
ConstraintViolationException}.
*/
void evaluate(Class<? extends AbstractType> valueType, Operator
relationType, String term, Object columnValue) throws
ConstraintViolationException;
Review Comment:
commented else where already, just repeat here. The interface can be updated
to
```suggestion
void evaluate(AbstractType<?> valueType, ByteBuffer value, Operator
relationType, String term) throws ConstraintViolationException;
```
##########
src/java/org/apache/cassandra/tcm/serialization/Version.java:
##########
@@ -51,6 +51,10 @@ public enum Version
* - PreInitialize includes datacenter (affects local serialization on
first CMS node only)
*/
V5(5),
+ /**
+ * CEP-42 - Constraints framework
+ */
Review Comment:
Can you enrich the javadoc to justify that V6 bump is required?
##########
src/java/org/apache/cassandra/cql3/Json.java:
##########
@@ -89,7 +89,6 @@ public Prepared prepareAndCollectMarkers(TableMetadata
metadata, Collection<Colu
boundNames.add(bindIndex, makeReceiver(metadata));
return new PreparedMarker(bindIndex, receivers);
}
-
Review Comment:
unrelated change. Please revert.
##########
src/java/org/apache/cassandra/cql3/constraints/LengthConstraint.java:
##########
@@ -56,43 +55,30 @@ public void evaluate(Class<? extends AbstractType>
valueType, Operator relationT
int valueLength = getValueSize(columnValue, valueType);
int sizeConstraint = Integer.parseInt(term);
- switch (relationType)
- {
- case EQ:
- if (valueLength != sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be exactly " + sizeConstraint);
- break;
- case NEQ:
- if (valueLength == sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be different from " + sizeConstraint);
- break;
- case GT:
- if (valueLength <= sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be larger than " + sizeConstraint);
- break;
- case LT:
- if (valueLength >= sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be smaller than " + sizeConstraint);
- break;
- case GTE:
- if (valueLength < sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be larger or equal than " + sizeConstraint);
- break;
- case LTE:
- if (valueLength > sizeConstraint)
- throw new ConstraintViolationException(columnName + "
value length should be smaller or equala than " + sizeConstraint);
- break;
- default:
- throw new ConstraintViolationException("Invalid relation type:
" + relationType);
- }
+ ByteBuffer buffera = ByteBufferUtil.bytes(valueLength);
+ ByteBuffer bufferb = ByteBufferUtil.bytes(sizeConstraint);
+
+ if (!relationType.isSatisfiedBy(Int32Type.instance, buffera, bufferb))
Review Comment:
nit: rename to `leftOperand` and `rightOperand`
##########
src/java/org/apache/cassandra/cql3/constraints/ScalarColumnConstraint.java:
##########
@@ -81,35 +84,12 @@ public void evaluate(Class<? extends AbstractType>
valueType, Object columnValue
throw new ConstraintViolationException(param + " and " + term + "
need to be numbers.");
}
- switch (relationType)
- {
- case EQ:
- if (Float.compare(columnValueNumber.floatValue(),
sizeConstraint) != 0)
- throw new ConstraintViolationException(param + " value
should be exactly " + sizeConstraint);
- break;
- case NEQ:
- if (Double.compare(columnValueNumber.floatValue(),
sizeConstraint) == 0)
- throw new ConstraintViolationException(param + " value
should be different from " + sizeConstraint);
- break;
- case GT:
- if (columnValueNumber.floatValue() <= sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be larger than " + sizeConstraint);
- break;
- case LT:
- if (columnValueNumber.floatValue() >= sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be smaller than " + sizeConstraint);
- break;
- case GTE:
- if (columnValueNumber.floatValue() < sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be larger or equal than " + sizeConstraint);
- break;
- case LTE:
- if (columnValueNumber.floatValue() > sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be smaller or equal than " + sizeConstraint);
- break;
- default:
- throw new ConstraintViolationException("Invalid relation type:
" + relationType);
- }
+ ByteBuffer buffera =
ByteBufferUtil.bytes(columnValueNumber.floatValue());
+ ByteBuffer bufferb = ByteBufferUtil.bytes(sizeConstraint);
+
+ if (!relationType.isSatisfiedBy(FloatType.instance, buffera, bufferb))
+ throw new ConstraintViolationException(columnValueNumber + " does
not satisfy length constraint. "
+ + sizeConstraint + " should
be " + relationType + ' ' + term);
}
Review Comment:
I do not think this constraint is to compare the size. It is to compare the
values. The error message looks wrong.
##########
src/java/org/apache/cassandra/cql3/constraints/ScalarColumnConstraint.java:
##########
@@ -81,35 +84,12 @@ public void evaluate(Class<? extends AbstractType>
valueType, Object columnValue
throw new ConstraintViolationException(param + " and " + term + "
need to be numbers.");
}
- switch (relationType)
- {
- case EQ:
- if (Float.compare(columnValueNumber.floatValue(),
sizeConstraint) != 0)
- throw new ConstraintViolationException(param + " value
should be exactly " + sizeConstraint);
- break;
- case NEQ:
- if (Double.compare(columnValueNumber.floatValue(),
sizeConstraint) == 0)
- throw new ConstraintViolationException(param + " value
should be different from " + sizeConstraint);
- break;
- case GT:
- if (columnValueNumber.floatValue() <= sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be larger than " + sizeConstraint);
- break;
- case LT:
- if (columnValueNumber.floatValue() >= sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be smaller than " + sizeConstraint);
- break;
- case GTE:
- if (columnValueNumber.floatValue() < sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be larger or equal than " + sizeConstraint);
- break;
- case LTE:
- if (columnValueNumber.floatValue() > sizeConstraint)
- throw new ConstraintViolationException(param + " value
should be smaller or equal than " + sizeConstraint);
- break;
- default:
- throw new ConstraintViolationException("Invalid relation type:
" + relationType);
- }
+ ByteBuffer buffera =
ByteBufferUtil.bytes(columnValueNumber.floatValue());
+ ByteBuffer bufferb = ByteBufferUtil.bytes(sizeConstraint);
+
+ if (!relationType.isSatisfiedBy(FloatType.instance, buffera, bufferb))
Review Comment:
It is good that the patch now reuses `org.apache.cassandra.cql3.Operator`. I
recall having comment on this. But it probably got lost.
The interface
`org.apache.cassandra.cql3.constraints.ColumnConstraint#evaluate` can be
updated to pass `ByteBuffer` instead of `Object columnValue`.
Note that
`org.apache.cassandra.db.marshal.AbstractType#checkConstraints(java.nio.ByteBuffer,
org.apache.cassandra.cql3.constraints.ColumnConstraints)`, decompose
`ByteBuffer` to concrete typed object, which is pointless since it requires
`ByteBuffer`s to compare.
Besides that, can you rename `buffera` to `leftOperand` and `bufferb` to
`rightOperand` for better clarity. This one is a nit.
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraint.java:
##########
@@ -20,16 +20,43 @@
import org.apache.cassandra.cql3.CqlBuilder;
import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.tcm.serialization.MetadataSerializer;
import org.apache.cassandra.schema.ColumnMetadata;
/**
* Common class for the conditions that a CQL Constraint needs to implement to
be integrated in the
* CQL Constraints framework.
*/
-public interface ColumnConstraint
+public interface ColumnConstraint<T>
Review Comment:
nit: add javadoc to explain what is type parameter T
##########
src/resources/org/apache/cassandra/cql3/reserved_keywords.txt:
##########
@@ -54,4 +54,5 @@ USE
USING
VIEW
WHERE
-WITH
\ No newline at end of file
+WITH
+CONSTRAINT
Review Comment:
Why adding `CONSTRAINT` as a reserved keyword, again? We had reached the
consensus of dropping `CONSTRAINT` as a reserved keyword.
https://lists.apache.org/thread/kd29rtnjo3o2nclr2qq25sz7p0wq3sx1
In fact, there is no such keyword `K_CONSTRAINT` in the current CQL grammar.
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraint.java:
##########
@@ -20,16 +20,43 @@
import org.apache.cassandra.cql3.CqlBuilder;
import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.tcm.serialization.MetadataSerializer;
import org.apache.cassandra.schema.ColumnMetadata;
/**
* Common class for the conditions that a CQL Constraint needs to implement to
be integrated in the
* CQL Constraints framework.
*/
-public interface ColumnConstraint
+public interface ColumnConstraint<T>
{
- IVersionedSerializer<ColumnConstraint> serializer();
+
+ // Enum containing all the possible constraint serializers to help with
serialization/deserialization
+ // of constraints.
+ enum ConstraintType
+ {
+ // The order of that enum matters!!
+ // We are serializing its enum position instead of its name.
+ // Changing this enum would affect how that int is interpreted when
deserializing.
Review Comment:
It is not strictly necessary that order matters here. The ser/deser code can
write the enum name instead, and lookup by the name, i.e.
`ConstraintType.valueOf(...)`.
Not advocating for the change. I guess it is mostly a FYI thing.
##########
src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java:
##########
@@ -582,6 +582,9 @@ public void validate(ClientState state)
public KeyspaceMetadata apply(Epoch epoch, KeyspaceMetadata keyspace,
TableMetadata table, ClusterMetadata metadata)
{
attrs.validate();
+ if (attrs.getBoolean(TableParams.Option.CDC.toString(), false) &&
table.hasConstraints)
+ throw new InvalidRequestException("CDC and Constraints should
not be enabled at the same time as" +
+ " it could cause issues with
node bootstraping");
Review Comment:
The validation prevents enabling CDC when there is constraint defined, or
the other way around as defined at line#759.
But I do not see how they cause issue with bootstrapping. Can you plain?
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraint.java:
##########
@@ -20,16 +20,43 @@
import org.apache.cassandra.cql3.CqlBuilder;
import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.tcm.serialization.MetadataSerializer;
import org.apache.cassandra.schema.ColumnMetadata;
/**
* Common class for the conditions that a CQL Constraint needs to implement to
be integrated in the
* CQL Constraints framework.
*/
-public interface ColumnConstraint
+public interface ColumnConstraint<T>
{
- IVersionedSerializer<ColumnConstraint> serializer();
+
+ // Enum containing all the possible constraint serializers to help with
serialization/deserialization
+ // of constraints.
+ enum ConstraintType
Review Comment:
I am neutral to using enum. I guess the motivation is to look up a
serializer by the ordinal number (int). How is it different from look up the
serializer by the name, which is the original implementation.
Both approaches are essentially the same. The only advantage is probably
slightly smaller serialized data.
##########
src/java/org/apache/cassandra/db/marshal/AbstractCompositeType.java:
##########
@@ -50,6 +50,12 @@ public boolean allowsEmpty()
return true;
}
+ @Override
+ public boolean isConstrainable()
+ {
+ return false;
Review Comment:
maybe add a comment to explain why it returns false for composite types.
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraint.java:
##########
@@ -20,16 +20,43 @@
import org.apache.cassandra.cql3.CqlBuilder;
import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.tcm.serialization.MetadataSerializer;
import org.apache.cassandra.schema.ColumnMetadata;
/**
* Common class for the conditions that a CQL Constraint needs to implement to
be integrated in the
* CQL Constraints framework.
*/
-public interface ColumnConstraint
+public interface ColumnConstraint<T>
{
- IVersionedSerializer<ColumnConstraint> serializer();
+
+ // Enum containing all the possible constraint serializers to help with
serialization/deserialization
+ // of constraints.
+ enum ConstraintType
+ {
+ // The order of that enum matters!!
+ // We are serializing its enum position instead of its name.
+ // Changing this enum would affect how that int is interpreted when
deserializing.
+ COMPOSED(ColumnConstraints.serializer),
+ FUNCTION(FunctionColumnConstraint.serializer),
+ SCALAR(ScalarColumnConstraint.serializer);
+
+ private static final ConstraintType[] values = ConstraintType.values();
Review Comment:
This is a redundant field. Can you just use `ConstraintType.values()` in
method `getSerializer(int i)`?
##########
src/java/org/apache/cassandra/schema/ColumnMetadata.java:
##########
@@ -363,7 +358,7 @@ public void setColumnConstraints(ColumnConstraints
constraints)
@Nullable
Review Comment:
Another annotation on return type can be removed due to void method.
##########
src/java/org/apache/cassandra/cql3/constraints/FunctionColumnConstraint.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * 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.cassandra.cql3.constraints;
+
+import java.io.IOException;
+
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.CqlBuilder;
+import org.apache.cassandra.cql3.Operator;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.utils.LocalizeString;
+
+public class FunctionColumnConstraint implements
ColumnConstraint<FunctionColumnConstraint>
+{
+ public static final Serializer serializer = new Serializer();
+
+ public final ConstraintFunction function;
+ public final ColumnIdentifier columnName;
+ public final Operator relationType;
+ public final String term;
+
+ public final static class Raw
+ {
+ public final ConstraintFunction function;
+ public final ColumnIdentifier columnName;
+ public final Operator relationType;
+ public final String term;
+
+ public Raw(ColumnIdentifier functionName, ColumnIdentifier columnName,
Operator relationType, String term)
+ {
+ this.relationType = relationType;
+ this.columnName = columnName;
+ this.term = term;
+ function = createConstraintFunction(functionName.toCQLString(),
columnName);
+ }
+
+ public FunctionColumnConstraint prepare()
+ {
+ return new FunctionColumnConstraint(function, columnName,
relationType, term);
+ }
+ }
+
+ private enum Function
+ {
+ LENGTH(LengthConstraint::new);
+
+ private final java.util.function.Function<ColumnIdentifier,
ConstraintFunction> functionCreator;
Review Comment:
Agreed with Josh. It is a _private_ enum in the end. Let's avoid clashing
with the public one in jdk.
My suggestion is to rename it to `Functions`.
##########
src/java/org/apache/cassandra/schema/ColumnMetadata.java:
##########
@@ -354,7 +349,7 @@ public ColumnConstraints getColumnConstraints()
return columnConstraints;
}
- @Nullable
+ @Nonnull
Review Comment:
The annotation is pointless. The return type is `void`.
##########
src/java/org/apache/cassandra/db/marshal/AbstractCompositeType.java:
##########
@@ -360,7 +360,7 @@ public void checkConstraints(ByteBuffer input,
ColumnConstraints constraints) th
{
// contraints list should have the exact size of partition key
values.
// Noop constraints are filled for the partition key columns w/o
any constraints.
- throw new IllegalStateException("Partition key values and number
of constraints do not match: "
+ throw new IllegalStateException("Partition key value number should
be same with number of constraints: "
+ partitionKeyValues.size() + "!="
+ constraints.getConstraints().size());
Review Comment:
nit: To make the error message a bit more clear, can you indicate the name
of the values (size)? The current message prints "1 != 2" for example. How
about "The number of constraints (value) should be the same with the number of
partition key columns (value)."?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
