yifan-c commented on code in PR #4411:
URL: https://github.com/apache/cassandra/pull/4411#discussion_r2466772709
##########
src/java/org/apache/cassandra/schema/ColumnMetadata.java:
##########
@@ -788,7 +831,8 @@ public long serializedSize(ColumnMetadata t, Version
version)
BOOL_SIZE +
((t.mask == null) ? 0 :
ColumnMask.serializer.serializedSize(t.mask, version)) +
constraintsSize +
- (version.isAtLeast(Version.V7) ? sizeofVInt(t.uniqueId) :
0);
+ (version.isAtLeast(Version.V7) ? sizeofVInt(t.uniqueId) :
0) +
+ (version.isAtLeast(Version.V8) ? (sizeof(t.comment) +
sizeof(t.securityLabel)) : 0);
Review Comment:
Seriously, should we split this long statement up? Those chained ternary
operators are better written in if-else for readability.
##########
src/java/org/apache/cassandra/schema/TableParams.java:
##########
@@ -335,6 +341,7 @@ public String toString()
{
return MoreObjects.toStringHelper(this)
.add(COMMENT.toString(), comment)
+ .add(Option.SECURITY_LABEL.toString(), securityLabel)
Review Comment:
nit: let it be consistent with the other lines? Please statically import
`SECURITY_LABEL`
##########
src/java/org/apache/cassandra/schema/KeyspaceParams.java:
##########
@@ -174,13 +204,21 @@ public KeyspaceParams deserialize(DataInputPlus in,
Version version) throws IOEx
FastPathStrategy fastPath = version.isAtLeast(MIN_ACCORD_VERSION)
? FastPathStrategy.serializer.deserialize(in, version)
: FastPathStrategy.simple();
- return new KeyspaceParams(durableWrites, params, fastPath);
+ String comment = EMPTY_COMMENT;
+ String securityLabel = EMPTY_SECURITY_LABEL;
+ if (version.isAtLeast(Version.V8))
+ {
+ comment = in.readUTF();
+ securityLabel = in.readUTF();
+ }
+ return new KeyspaceParams(durableWrites, params, fastPath,
comment, securityLabel);
}
public long serializedSize(KeyspaceParams t, Version version)
{
return ReplicationParams.serializer.serializedSize(t.replication,
version) +
TypeSizes.sizeof(t.durableWrites) +
+ (version.isAtLeast(Version.V8) ?
(TypeSizes.sizeof(t.comment) + TypeSizes.sizeof(t.securityLabel)) : 0) +
(version.isAtLeast(MIN_ACCORD_VERSION) ?
FastPathStrategy.serializer.serializedSize(t.fastPath, version) : 0);
Review Comment:
nit: split the long single statement up. It is fine so far, but would be
good if we do it early.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
Review Comment:
`writeUnsignedVInt32`?
##########
src/java/org/apache/cassandra/schema/TableParams.java:
##########
@@ -375,6 +382,8 @@ public void appendCqlTo(CqlBuilder builder, boolean isView)
.newLine()
.append("AND cdc = ").append(cdc)
.newLine()
+ // TODO: AND comment should be deprecatod in future releases in
favor of
+ // JIRA Introducing comments and security labels for schema
elements
.append("AND comment = ").appendWithSingleQuotes(comment)
Review Comment:
Could you file a JIRA and update the TODO here? The Comment statement is
introduced in Cassandra 6.0, and it could be a good idea to drop the comment
table attribute in 7.0.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
Review Comment:
why use the full qualified class name?
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
Review Comment:
should `metadataGetter` just return `null`, instead of `getOrDefault` and
return empty string? I do not see the value of returning an empty string here.
It just adds a few more instruments to process.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
+ {
+ out.writeInt(i);
+ out.writeUTF(value);
+ }
+ }
+ }
+
+ private void deserializeFieldMetadata(DataInputPlus in, List<String>
fieldComments, List<String> fieldSecurityLabels) throws IOException
+ {
+ deserializeSparseMetadata(in, fieldComments);
+ deserializeSparseMetadata(in, fieldSecurityLabels);
+ }
+
+ private void deserializeSparseMetadata(DataInputPlus in, List<String>
target) throws IOException
+ {
+ int count = in.readInt();
+ for (int i = 0; i < count; i++)
+ {
+ int position = in.readInt();
+ String value = in.readUTF();
+ // Ensure the list is large enough to accommodate this position
+ while (target.size() <= position)
+ target.add("");
Review Comment:
I do not think this is needed.
You know the number of all fields already in
`org.apache.cassandra.schema.Types.Serializer#deserialize` before calling this
method. You can create the list with the exact size, `new
ArrayList<>(fieldNamesSize)` and update the field at the position in
`deserializeSparseMetadata`
##########
src/java/org/apache/cassandra/db/marshal/UserType.java:
##########
@@ -442,14 +474,66 @@ public UserType withUpdatedUserType(UserType udt)
{
return isMultiCell == udt.isMultiCell
? udt
- : new UserType(keyspace, name, udt.fieldNames(),
udt.fieldTypes(), isMultiCell);
+ : new UserType(keyspace, name, udt.fieldNames(),
udt.fieldTypes(), isMultiCell, udt.comment, udt.securityLabel,
udt.fieldComments, udt.fieldSecurityLabels);
}
return new UserType(keyspace,
name,
fieldNames,
Lists.newArrayList(transform(fieldTypes(), t ->
t.withUpdatedUserType(udt))),
- isMultiCell());
+ isMultiCell(),
+ comment,
+ securityLabel,
+ fieldComments,
+ fieldSecurityLabels);
+ }
+
+ public UserType withComment(String comment)
+ {
+ return new UserType(keyspace, name, fieldNames, fieldTypes(),
isMultiCell(), comment, securityLabel, fieldComments, fieldSecurityLabels);
+ }
+
+ public UserType withSecurityLabel(String securityLabel)
+ {
+ return new UserType(keyspace, name, fieldNames, fieldTypes(),
isMultiCell(), comment, securityLabel, fieldComments, fieldSecurityLabels);
+ }
+
+ public UserType withFieldComment(FieldIdentifier fieldName, String
fieldComment)
+ {
+ if (fieldPosition(fieldName) == -1)
+ throw new IllegalArgumentException(String.format("Field '%s'
doesn't exist in type '%s.%s'", fieldName, keyspace, getNameAsString()));
+
+ Map<FieldIdentifier, String> newFieldComments = new
HashMap<>(fieldComments);
+ if (fieldComment == null || fieldComment.isEmpty())
+ newFieldComments.remove(fieldName);
Review Comment:
field comment is removed when user sets the comment to empty string? In the
example, one only removes the comment by setting the comment to `NULL`.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
+ {
+ out.writeInt(i);
+ out.writeUTF(value);
+ }
+ }
+ }
+
+ private void deserializeFieldMetadata(DataInputPlus in, List<String>
fieldComments, List<String> fieldSecurityLabels) throws IOException
+ {
+ deserializeSparseMetadata(in, fieldComments);
+ deserializeSparseMetadata(in, fieldSecurityLabels);
+ }
+
+ private void deserializeSparseMetadata(DataInputPlus in, List<String>
target) throws IOException
+ {
+ int count = in.readInt();
+ for (int i = 0; i < count; i++)
+ {
+ int position = in.readInt();
+ String value = in.readUTF();
+ // Ensure the list is large enough to accommodate this position
+ while (target.size() <= position)
+ target.add("");
+ target.set(position, value);
+ }
+ }
+
+ private long fieldMetadataSize(UserType type, List<FieldIdentifier>
fieldIdentifiers)
+ {
+ return sparseMetadataSize(fieldIdentifiers, type::fieldComment)
+ + sparseMetadataSize(fieldIdentifiers,
type::fieldSecurityLabel);
+ }
+
+ private long sparseMetadataSize(List<FieldIdentifier> fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter)
+ {
+ int nonEmptyCount = 0;
+ long dataSize = 0;
+
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ String value = metadataGetter.apply(fieldId);
+ if (!value.isEmpty())
+ {
+ nonEmptyCount++;
+ dataSize += sizeof(0); // position (int)
Review Comment:
update to use `sizeofUnsignedVInt` when using vint elsewhere.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
+ {
+ out.writeInt(i);
Review Comment:
hmm, I think it does not need to use `int` in practice. However, there is
not restriction on limit the number of fields in a UDT. So theoretically, `int`
is required. One can go really crazy and create Integer.MAX_VALUE number of
fields.
Add restriction is out of the scope of this PR.
But I think you might want to write the variable int to avoid write bunch 0s.
```suggestion
out.writeUnsignedVInt32(i);
```
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
+ {
+ out.writeInt(i);
+ out.writeUTF(value);
+ }
+ }
+ }
+
+ private void deserializeFieldMetadata(DataInputPlus in, List<String>
fieldComments, List<String> fieldSecurityLabels) throws IOException
+ {
+ deserializeSparseMetadata(in, fieldComments);
+ deserializeSparseMetadata(in, fieldSecurityLabels);
+ }
+
+ private void deserializeSparseMetadata(DataInputPlus in, List<String>
target) throws IOException
+ {
+ int count = in.readInt();
Review Comment:
`readUnsignedVInt32`?
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
Review Comment:
You should be able to avoid the second evaluation. It is a waste.
##########
doc/modules/cassandra/pages/developing/cql/ddl.adoc:
##########
@@ -801,3 +801,195 @@ statements.
However, tables are the only object that can be truncated currently, and the
`TABLE` keyword can be omitted.
Truncating a table permanently removes all existing data from the table, but
without removing the table itself.
+
+[[comment-statement]]
+== COMMENT ON
+
+The `COMMENT ON` statement allows you to add descriptive comments to schema
elements for documentation purposes.
+Comments are stored in the schema metadata and displayed when using `DESCRIBE`
statements.
+
+=== COMMENT ON KEYSPACE
+
+Add or modify a comment on a keyspace:
+
+[source,cql]
+----
+COMMENT ON KEYSPACE keyspace_name IS 'comment text';
+COMMENT ON KEYSPACE keyspace_name IS NULL; -- Remove comment
+----
+
+Example:
+
+[source,cql]
+----
+COMMENT ON KEYSPACE cycling IS 'Keyspace for cycling application data';
+----
+
+=== COMMENT ON TABLE
+
+Add or modify a comment on a table:
+
+[source,cql]
+----
+COMMENT ON TABLE [keyspace_name.]table_name IS 'comment text';
+COMMENT ON TABLE [keyspace_name.]table_name IS NULL; -- Remove comment
+----
+
+Example:
+
+[source,cql]
+----
+COMMENT ON TABLE cycling.cyclist_name IS 'Table storing cyclist names and
basic information';
+----
+
+=== COMMENT ON COLUMN
+
+Add or modify a comment on a column:
+
+[source,cql]
+----
+COMMENT ON COLUMN [keyspace_name.]table_name.column_name IS 'comment text';
+COMMENT ON COLUMN [keyspace_name.]table_name.column_name IS NULL; -- Remove
comment
+----
Review Comment:
Once the comment is removed, describe table should not print the `COMMENT ON
COLUMN` statement.
##########
src/java/org/apache/cassandra/schema/Types.java:
##########
@@ -498,16 +570,101 @@ public long serializedSize(Types t, Version version)
for (UserType type : t.types.values())
{
size += sizeof(type.getNameAsString());
- List<String> fieldNames =
type.fieldNames().stream().map(FieldIdentifier::toString).collect(toList());
- List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());
+ List<FieldIdentifier> fieldIdentifiers = type.fieldNames();
+ List<String> fieldNames =
fieldIdentifiers.stream().map(FieldIdentifier::toString).collect(toList());
+ List<String> fieldTypes =
type.fieldTypes().stream().map(AbstractType::asCQL3Type).map(CQL3Type::toString).collect(toList());;
size += sizeof(fieldNames.size());
for (String s : fieldNames)
size += sizeof(s);
size += sizeof(fieldTypes.size());
for (String s : fieldTypes)
size += sizeof(s);
+ if (version.isAtLeast(Version.V8))
+ {
+ size += sizeof(type.comment);
+ size += sizeof(type.securityLabel);
+ size += fieldMetadataSize(type, fieldIdentifiers);
+ }
}
return size;
}
+
+ private void serializeFieldMetadata(UserType type,
List<FieldIdentifier> fieldIdentifiers, DataOutputPlus out) throws IOException
+ {
+ // Sparse serialization: only serialize non-empty metadata with
their positions
+ serializeSparseMetadata(fieldIdentifiers, type::fieldComment, out);
+ serializeSparseMetadata(fieldIdentifiers,
type::fieldSecurityLabel, out);
+ }
+
+ private void serializeSparseMetadata(List<FieldIdentifier>
fieldIdentifiers,
+
java.util.function.Function<FieldIdentifier, String> metadataGetter,
+ DataOutputPlus out) throws
IOException
+ {
+ // Count non-empty values
+ int nonEmptyCount = 0;
+ for (FieldIdentifier fieldId : fieldIdentifiers)
+ {
+ if (!metadataGetter.apply(fieldId).isEmpty())
+ nonEmptyCount++;
+ }
+
+ // Write count followed by position-value pairs
+ out.writeInt(nonEmptyCount);
+ for (int i = 0; i < fieldIdentifiers.size(); i++)
+ {
+ String value = metadataGetter.apply(fieldIdentifiers.get(i));
+ if (!value.isEmpty())
+ {
+ out.writeInt(i);
+ out.writeUTF(value);
+ }
+ }
+ }
+
+ private void deserializeFieldMetadata(DataInputPlus in, List<String>
fieldComments, List<String> fieldSecurityLabels) throws IOException
+ {
+ deserializeSparseMetadata(in, fieldComments);
+ deserializeSparseMetadata(in, fieldSecurityLabels);
+ }
+
+ private void deserializeSparseMetadata(DataInputPlus in, List<String>
target) throws IOException
+ {
+ int count = in.readInt();
+ for (int i = 0; i < count; i++)
+ {
+ int position = in.readInt();
Review Comment:
Update here too, if you have updated the serialization method.
```suggestion
int position = in.readUnsignedVInt32();
```
##########
src/java/org/apache/cassandra/cql3/ColumnSpecification.java:
##########
@@ -28,17 +28,28 @@
public class ColumnSpecification
{
+ protected static final String EMPTY_COMMENT = "";
+ protected static final String EMPTY_SECURITY_LABEL = "";
public final String ksName;
public final String cfName;
public final ColumnIdentifier name;
public final AbstractType<?> type;
+ public final String comment;
+ public final String securityLabel;
public ColumnSpecification(String ksName, String cfName, ColumnIdentifier
name, AbstractType<?> type)
+ {
+ this(ksName, cfName, name, type, EMPTY_COMMENT, EMPTY_SECURITY_LABEL);
+ }
+
+ public ColumnSpecification(String ksName, String cfName, ColumnIdentifier
name, AbstractType<?> type, String comment, String securityLabel)
{
this.ksName = ksName;
this.cfName = cfName;
this.name = name;
this.type = type;
+ this.comment = comment == null ? EMPTY_COMMENT : comment;
+ this.securityLabel = securityLabel == null ? EMPTY_SECURITY_LABEL :
securityLabel;
Review Comment:
Empty string is assigned when the value is null. Then, how do un-setting
comment and security label queries work?
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -1050,6 +1051,201 @@ public void
testDescFunctionAndAggregateShouldNotOmitQuotations() throws Throwab
}
}
+ @Test
+ public void testDescribeKeyspaceWithCommentsAndSecurityLabels() throws
Throwable
+ {
+ try
+ {
+ // Create keyspace with comment and security label
+ execute("CREATE KEYSPACE test_comments WITH REPLICATION = {'class'
: 'SimpleStrategy', 'replication_factor' : 1}");
+ execute("COMMENT ON KEYSPACE test_comments IS 'This is a test
keyspace with comments'");
+ execute("SECURITY LABEL ON KEYSPACE test_comments IS
'confidential'");
+
+ String expectedKeyspaceOutput = "CREATE KEYSPACE test_comments
WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}" +
+ " AND durable_writes = true" +
+ " AND fast_path = 'simple';\n" +
+ "COMMENT ON KEYSPACE test_comments
IS 'This is a test keyspace with comments';\n" +
+ "SECURITY LABEL ON KEYSPACE
test_comments IS 'confidential';";
+
+ for (String describeKeyword : new String[]{ "DESCRIBE", "DESC" })
+ {
+ assertRowsNet(executeDescribeNet(describeKeyword + " ONLY
KEYSPACE test_comments"),
+ row("test_comments", "keyspace",
"test_comments", expectedKeyspaceOutput));
+ }
+ }
+ finally
+ {
+ execute("DROP KEYSPACE IF EXISTS test_comments");
+ }
+ }
+
+ @Test
+ public void testDescribeTableWithCommentsAndSecurityLabels() throws
Throwable
+ {
+ try
+ {
+ // Create keyspace and table with comments and security labels
+ execute("CREATE KEYSPACE test_table_comments WITH REPLICATION =
{'class' : 'SimpleStrategy', 'replication_factor' : 1}");
+ execute("CREATE TABLE test_table_comments.users (id int PRIMARY
KEY, name text, email text)");
+
+ // Add comments and security labels to table and columns
+ execute("COMMENT ON TABLE test_table_comments.users IS 'User
information table'");
+ execute("SECURITY LABEL ON TABLE test_table_comments.users IS
'restricted'");
+ execute("COMMENT ON COLUMN test_table_comments.users.name IS 'User
full name'");
+ execute("SECURITY LABEL ON COLUMN test_table_comments.users.name
IS 'pii'");
+
+ String expectedTableOutput = "CREATE TABLE
test_table_comments.users (\n" +
+ " id int PRIMARY KEY,\n" +
+ " email text,\n" +
+ " name text\n" +
+ ") WITH " + tableParametersCql("User
information table") + "\n" +
+ "COMMENT ON TABLE
test_table_comments.users IS 'User information table';\n" +
+ "SECURITY LABEL ON TABLE
test_table_comments.users IS 'restricted';\n" +
+ "COMMENT ON COLUMN
test_table_comments.users.name IS 'User full name';\n" +
+ "SECURITY LABEL ON COLUMN
test_table_comments.users.name IS 'pii';";
+ for (String describeKeyword : new String[]{ "DESCRIBE", "DESC" })
+ {
+ assertRowsNet(executeDescribeNet("test_table_comments",
describeKeyword + " TABLE users"),
+ row("test_table_comments", "table", "users",
expectedTableOutput));
+ }
+ }
+ finally
+ {
+ execute("DROP KEYSPACE IF EXISTS test_table_comments");
+ }
+ }
+
+ @Test
+ public void testDescribeUserTypeWithCommentsAndSecurityLabels() throws
Throwable
+ {
+ try
+ {
+ // Create a user-defined type with comments and security labels
+ String type = createType(KEYSPACE_PER_TEST, "CREATE TYPE %s (name
text, age int, address text)");
+
+ execute("COMMENT ON TYPE " + KEYSPACE_PER_TEST + "." + type + " IS
'Person information type'");
+ execute("SECURITY LABEL ON TYPE " + KEYSPACE_PER_TEST + "." + type
+ " IS 'personal_data'");
+
+ String expectedTypeOutput = "CREATE TYPE " + KEYSPACE_PER_TEST +
"." + type + " (\n" +
+ " name text,\n" +
+ " age int,\n" +
+ " address text\n" +
+ ");\n" +
+ "COMMENT ON TYPE " + KEYSPACE_PER_TEST
+ "." + type + " IS 'Person information type';\n" +
+ "SECURITY LABEL ON TYPE " +
KEYSPACE_PER_TEST + "." + type + " IS 'personal_data';";
+
+ for (String describeKeyword : new String[]{ "DESCRIBE", "DESC" })
+ {
+ assertRowsNet(executeDescribeNet(KEYSPACE_PER_TEST,
describeKeyword + " TYPE " + type),
+ row(KEYSPACE_PER_TEST, "type", type,
expectedTypeOutput));
+ }
+ }
+ finally
+ {
+ // Cleanup is handled by the base test class
+ }
+ }
+
+ @Test
+ public void testDescribeUserTypeWithFieldCommentsAndSecurityLabels()
throws Throwable
+ {
+ try
+ {
+ // Create a user-defined type with field-level comments and
security labels
+ String type = createType(KEYSPACE_PER_TEST, "CREATE TYPE %s
(latitude double, longitude double)");
+
+ execute("COMMENT ON TYPE " + KEYSPACE_PER_TEST + "." + type + " IS
'Geographic location'");
+ execute("SECURITY LABEL ON TYPE " + KEYSPACE_PER_TEST + "." + type
+ " IS 'GEODATA'");
+ execute("COMMENT ON FIELD " + KEYSPACE_PER_TEST + "." + type +
".latitude IS 'Latitude in decimal degrees'");
+ execute("SECURITY LABEL ON FIELD " + KEYSPACE_PER_TEST + "." +
type + ".latitude IS 'SENSITIVE'");
+ execute("COMMENT ON FIELD " + KEYSPACE_PER_TEST + "." + type +
".longitude IS 'Longitude in decimal degrees'");
+ execute("SECURITY LABEL ON FIELD " + KEYSPACE_PER_TEST + "." +
type + ".longitude IS 'SENSITIVE'");
+
+ String expectedTypeOutput = "CREATE TYPE " + KEYSPACE_PER_TEST +
"." + type + " (\n" +
+ " latitude double,\n" +
+ " longitude double\n" +
+ ");\n" +
+ "COMMENT ON TYPE " + KEYSPACE_PER_TEST
+ "." + type + " IS 'Geographic location';\n" +
+ "COMMENT ON FIELD " + KEYSPACE_PER_TEST
+ "." + type + ".latitude IS 'Latitude in decimal degrees';\n" +
+ "COMMENT ON FIELD " + KEYSPACE_PER_TEST
+ "." + type + ".longitude IS 'Longitude in decimal degrees';\n" +
+ "SECURITY LABEL ON TYPE " +
KEYSPACE_PER_TEST + "." + type + " IS 'GEODATA';\n" +
+ "SECURITY LABEL ON FIELD " +
KEYSPACE_PER_TEST + "." + type + ".latitude IS 'SENSITIVE';\n" +
+ "SECURITY LABEL ON FIELD " +
KEYSPACE_PER_TEST + "." + type + ".longitude IS 'SENSITIVE';";
+
+ for (String describeKeyword : new String[]{ "DESCRIBE", "DESC" })
+ {
+ assertRowsNet(executeDescribeNet(KEYSPACE_PER_TEST,
describeKeyword + " TYPE " + type),
+ row(KEYSPACE_PER_TEST, "type", type,
expectedTypeOutput));
+ }
+ }
+ finally
+ {
+ // Cleanup is handled by the base test class
+ }
+ }
+
+ @Test
+ public void testDescribeSchemaWithCommentsAndSecurityLabels() throws
Throwable
+ {
+ try
+ {
+ // Create a comprehensive schema with comments and security labels
+ execute("CREATE KEYSPACE test_schema WITH REPLICATION = {'class' :
'SimpleStrategy', 'replication_factor' : 1}");
+ execute("COMMENT ON KEYSPACE test_schema IS 'Test schema for
comments and security labels'");
+ execute("SECURITY LABEL ON KEYSPACE test_schema IS
'test_environment'");
+
+ execute("CREATE TYPE test_schema.address_type (street text, city
text, zip text)");
+ String type = "address_type";
+ execute("COMMENT ON TYPE test_schema." + type + " IS 'Address
information'");
+ execute("SECURITY LABEL ON TYPE test_schema." + type + " IS
'location_data'");
+
+ execute("CREATE TABLE test_schema.customers (id int PRIMARY KEY,
name text, addr " + type + ")");
+ execute("COMMENT ON TABLE test_schema.customers IS 'Customer
information'");
+ execute("SECURITY LABEL ON TABLE test_schema.customers IS
'customer_data'");
+ execute("COMMENT ON COLUMN test_schema.customers.name IS 'Customer
name'");
+ execute("SECURITY LABEL ON COLUMN test_schema.customers.name IS
'pii'");
+
+ ResultSet result = executeDescribeNet("DESCRIBE SCHEMA");
+ boolean foundKeyspaceWithComment = false;
+ boolean foundTypeWithComment = false;
+ boolean foundTableWithComment = false;
+
+ for (Row row : result.all())
+ {
+ String createStatement = row.getString("create_statement");
+
+ if (row.getString("type").equals("keyspace") &&
row.getString("name").equals("test_schema"))
+ {
+ assertTrue("Keyspace should include comment",
createStatement.contains("COMMENT ON KEYSPACE test_schema IS 'Test schema for
comments and security labels'"));
+ assertTrue("Keyspace should include security label",
createStatement.contains("SECURITY LABEL ON KEYSPACE test_schema IS
'test_environment'"));
+ foundKeyspaceWithComment = true;
+ }
+ else if (row.getString("type").equals("type") &&
row.getString("name").equals(type))
+ {
+ assertTrue("Type should include comment",
createStatement.contains("COMMENT ON TYPE test_schema." + type + " IS 'Address
information'"));
+ assertTrue("Type should include security label",
createStatement.contains("SECURITY LABEL ON TYPE test_schema." + type + " IS
'location_data'"));
+ foundTypeWithComment = true;
+ }
+ else if (row.getString("type").equals("table") &&
row.getString("name").equals("customers"))
+ {
+ assertTrue("Table should include comment",
createStatement.contains("COMMENT ON TABLE test_schema.customers IS 'Customer
information';"));
+ assertTrue("Table should include security label",
createStatement.contains("SECURITY LABEL ON TABLE test_schema.customers IS
'customer_data'"));
+ assertTrue("Table should include column comment",
createStatement.contains("COMMENT ON COLUMN test_schema.customers.name IS
'Customer name'"));
+ assertTrue("Table should include column security label",
createStatement.contains("SECURITY LABEL ON COLUMN test_schema.customers.name
IS 'pii'"));
+ foundTableWithComment = true;
+ }
+ }
+
+ assertTrue("Should find keyspace with comment in schema",
foundKeyspaceWithComment);
+ assertTrue("Should find type with comment in schema",
foundTypeWithComment);
+ assertTrue("Should find table with comment in schema",
foundTableWithComment);
+ }
+ finally
+ {
+ execute("DROP KEYSPACE IF EXISTS test_schema");
+ }
+ }
+
Review Comment:
Please add test scenarios.
1. `DESCRIBE` after `comment|security label` is unset
2. Make `comment|security label` on keyspace|table|column|etc.` and drop the
`keyspace|table|column|etc.` and run `DESCRIBE`
Both test scenarios should not print the `comment|security label` statements.
##########
src/java/org/apache/cassandra/schema/TableMetadata.java:
##########
@@ -1482,6 +1488,32 @@ public Builder alterColumnMask(ColumnIdentifier name,
@Nullable ColumnMask mask)
return this;
}
+ public Builder alterColumnComment(ColumnIdentifier name, String
comment)
+ {
+ ColumnMetadata column = columns.get(name.bytes);
+ if (column == null)
+ throw new IllegalArgumentException("Column " + name + " is
invalid");
Review Comment:
Why it is invalid? Should it say the column does not exist?
--
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]
