yifan-c commented on code in PR #3562:
URL: https://github.com/apache/cassandra/pull/3562#discussion_r1909618503
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraints.java:
##########
@@ -78,21 +96,25 @@ public boolean isEmpty()
return constraints.isEmpty();
}
-
@Override
- public void validate(ColumnMetadata columnMetadata, TableMetadata
tableMetadata) throws InvalidConstraintDefinitionException
+ public void validate(ColumnMetadata columnMetadata) throws
InvalidConstraintDefinitionException
{
+ if (complexTypes.contains(columnMetadata.type))
+ {
+ throw new InvalidConstraintDefinitionException("Complex types do
not support constraints");
+ }
Review Comment:
I think you want to check with the class, not the `AbstractType` object. I
also enriched the error message a bit
```suggestion
if (complexTypes.contains(columnMetadata.type.getClass()))
{
throw new InvalidConstraintDefinitionException("Constraint
cannot be defined on column '"
+
columnMetadata.name + "' with type: "
+
columnMetadata.type.asCQL3Type());
}
```
##########
src/java/org/apache/cassandra/cql3/constraints/ColumnConstraints.java:
##########
@@ -18,24 +18,42 @@
package org.apache.cassandra.cql3.constraints;
-
import org.apache.cassandra.cql3.CqlBuilder;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CompositeType;
+import org.apache.cassandra.db.marshal.DynamicCompositeType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.TupleType;
+import org.apache.cassandra.db.marshal.UserType;
import org.apache.cassandra.io.IVersionedAsymmetricSerializer;
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.schema.TableMetadata;
+import org.apache.cassandra.transport.DataType;
+import org.apache.cassandra.transport.ProtocolVersion;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
// group of constraints for the column
public class ColumnConstraints implements ColumnConstraint
{
- public static Serializer serializer = new Serializer();
+ public static final Serializer serializer = new Serializer();
+
+ private static final Set<Class<? extends AbstractType>> complexTypes =
ImmutableSet.of(MapType.class,
Review Comment:
`complex` means differently. `columnMetadata.isComplex() == true` indicate
the value type is collection and UDT.
Let's not override the meaning, which can create confusion for future code
readers.
How about just calling it `UNSUPPORTED_TYPES`? (All in upper-case).
##########
src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java:
##########
@@ -816,9 +818,25 @@ final void addUpdates(UpdatesCollector collector,
}
else
{
+ // Clustering keys need to be checked on their own
for (Clustering<?> clustering : clusterings)
{
clustering.validate();
+ for (int i = 0; i < clustering.size(); i++)
+ {
+ ColumnMetadata column =
metadata.clusteringColumns().get(i);
+ if (column.hasConstraint())
+ {
+ try
+ {
+ clustering.checkConstraints(i,
metadata.comparator, column.getColumnConstraints());
+ }
+ catch (ConstraintViolationException e)
+ {
+ throw new
InvalidRequestException(e.getMessage(), e);
+ }
+ }
+ }
Review Comment:
nit: extract out to a dedicated method, since the method `addUpdates` is
quite large already.
##########
src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java:
##########
@@ -89,6 +89,16 @@ public final class StatementRestrictions
*/
private PartitionKeyRestrictions partitionKeyRestrictions;
+ public List<ColumnMetadata> getClusteringColumnsRestrictions()
+ {
+ return clusteringColumnsRestrictions.restrictions.columns();
+ }
+
+ public List<ColumnMetadata> getPartitionKeyRestrictions()
+ {
+ return partitionKeyRestrictions.restrictions.columns();
+ }
+
Review Comment:
Those 2 methods are not used. Can we remove them?
##########
src/java/org/apache/cassandra/cql3/Validation.java:
##########
@@ -76,18 +78,18 @@ public static void
validateKeyAndCheckConstraints(TableMetadata metadata, ByteBu
validateKey(metadata, key);
List<ColumnMetadata> partitionKeys = metadata.partitionKeyColumns();
- List<ColumnConstraint> columnConstraints = new
ArrayList<>(partitionKeys.size());
+ List<ColumnConstraint> partitionKeyConstraints = new
ArrayList<>(partitionKeys.size());
Review Comment:
👍 more clear with this name
##########
src/java/org/apache/cassandra/cql3/Validation.java:
##########
@@ -76,18 +78,18 @@ public static void
validateKeyAndCheckConstraints(TableMetadata metadata, ByteBu
validateKey(metadata, key);
List<ColumnMetadata> partitionKeys = metadata.partitionKeyColumns();
- List<ColumnConstraint> columnConstraints = new
ArrayList<>(partitionKeys.size());
+ List<ColumnConstraint> partitionKeyConstraints = new
ArrayList<>(partitionKeys.size());
for (ColumnMetadata column : partitionKeys)
{
- columnConstraints.add(column.getColumnConstraints());
+ partitionKeyConstraints.add(column.getColumnConstraints());
Review Comment:
How about skipping the columns that do not have constraints? It should
minimize the list and exit early when no constraint is defined for partition
key, avoiding creating temporary objects.
--
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]
