beobal commented on code in PR #3921:
URL: https://github.com/apache/cassandra/pull/3921#discussion_r1971197366
##########
src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java:
##########
@@ -783,7 +784,6 @@ private enum Kind
RENAME_COLUMNS,
ALTER_OPTIONS,
DROP_COMPACT_STORAGE,
- DROP_CONSTRAINTS,
Review Comment:
Can you also remove the `DropConstraints` class please?
##########
src/antlr/Parser.g:
##########
@@ -980,7 +980,7 @@ alterTableStatement returns [AlterTableStatement.Raw stmt]
| K_ALTER ( K_IF K_EXISTS { $stmt.ifColumnExists(true); } )? id=cident
( mask=columnMask { $stmt.mask(id, mask); }
| K_DROP K_MASKED { $stmt.mask(id, null); }
- | K_DROP K_CHECK { $stmt.dropConstraints(id); }
+ | K_DROP K_CHECK { $stmt.alterConstraints(id, null); }
| (constraints=columnConstraints) { $stmt.alterConstraints(id,
constraints); })
Review Comment:
It isn't new with this patch but to be consistent with other similar methods
(e.g. `mask/add/drop/rename`), I would suggest renaming `alterConstraints` to
just `constrain`
##########
src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java:
##########
@@ -739,9 +739,9 @@ public KeyspaceMetadata apply(Epoch epoch, KeyspaceMetadata
keyspace, TableMetad
public static class AlterConstraints extends AlterTableStatement
{
final ColumnIdentifier columnName;
- final ColumnConstraints constraints;
+ final ColumnConstraints.Raw constraints;
- AlterConstraints(String keyspaceName, String tableName, boolean
ifTableExists, ColumnIdentifier columnName, ColumnConstraints constraints)
+ AlterConstraints(String keyspaceName, String tableName, boolean
ifTableExists, ColumnIdentifier columnName, ColumnConstraints.Raw constraints)
Review Comment:
This drops the `IF EXISTS` for the column, e.g.:
```
CREATE TABLE ks.t1 (k int PRIMARY KEY, a int);
ALTER TABLE ks.t1 ALTER IF EXISTS b CHECK b > 0;
```
should not error because `b` doesn't exist
##########
src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java:
##########
@@ -739,9 +739,9 @@ public KeyspaceMetadata apply(Epoch epoch, KeyspaceMetadata
keyspace, TableMetad
public static class AlterConstraints extends AlterTableStatement
Review Comment:
As with the grammar, although it isn't new to this patch (but it is
obviously a whole new feature) I would suggest renaming this to
`ConstrainColumn` to be more consistent with
`MaskColumn/DropColumn/AddColumns/etc`
##########
src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java:
##########
@@ -751,24 +751,25 @@ public static class AlterConstraints extends
AlterTableStatement
@Override
public KeyspaceMetadata apply(Epoch epoch, KeyspaceMetadata keyspace,
TableMetadata table, ClusterMetadata metadata)
{
- TableMetadata.Builder tableBuilder = table.unbuild().epoch(epoch);
-
- for (ColumnMetadata column : tableBuilder.columns())
+ for (ColumnMetadata column : table.columns())
Review Comment:
Why iterate here instead of just using `table.getColumn(columnName)`?
--
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]
