You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/01/11 16:01:21 UTC
[GitHub] [cassandra] blerer commented on a change in pull request #1319: modify alter statements with IF EXISTS and IF NOT EXISTS
blerer commented on a change in pull request #1319:
URL: https://github.com/apache/cassandra/pull/1319#discussion_r782269901
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java
##########
@@ -116,8 +122,8 @@ private void validateNoRangeMovements()
return;
Stream<InetAddressAndPort> unreachableNotAdministrativelyInactive =
- Gossiper.instance.getUnreachableMembers().stream().filter(endpoint -> !FBUtilities.getBroadcastAddressAndPort().equals(endpoint) &&
- !Gossiper.instance.isAdministrativelyInactiveState(endpoint));
+ Gossiper.instance.getUnreachableMembers().stream().filter(endpoint -> !FBUtilities.getBroadcastAddressAndPort().equals(endpoint) &&
Review comment:
You should keep the original formatting. We tend to avoid changing the formatting without good reasons has it makes the merging of patches accross branch more painful.
##########
File path: src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
##########
@@ -62,7 +65,9 @@ public void validate(ClientState state) throws RequestValidationException
// validate login here before authorize to avoid leaking user existence to anonymous users.
state.ensureNotAnonymous();
if (!DatabaseDescriptor.getRoleManager().isExistingRole(role))
- throw new InvalidRequestException(String.format("%s doesn't exist", role.getRoleName()));
+ {
+ RequestValidations.checkTrue(ifExists, String.format("Role %s doesn't exist", role.getRoleName()));
Review comment:
No need to use a `String.format` You can use simply use:
`checkTrue(ifExists, "Role %s doesn't exist", role.getRoleName());`
You can also use a static import for `checkTrue`
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -247,34 +260,40 @@ private void addColumn(KeyspaceMetadata keyspace,
}
/**
- * ALTER TABLE <table> DROP <column>
- * ALTER TABLE <table> DROP ( <column>, <column1>, ... <columnn>)
+ * ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] <column>
+ * ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] ( <column>, <column1>, ... <columnn>)
*/
// TODO: swap UDT refs with expanded tuples on drop
private static class DropColumns extends AlterTableStatement
{
private final Set<ColumnIdentifier> removedColumns;
+ private final boolean ifColumnExists;
private final Long timestamp;
- private DropColumns(String keyspaceName, String tableName, Set<ColumnIdentifier> removedColumns, Long timestamp)
+ private DropColumns(String keyspaceName, String tableName, Set<ColumnIdentifier> removedColumns, boolean ifColumnExists, Long timestamp, boolean ifTableExists)
Review comment:
nit: I would keep the `timestamp` first then `ifTableExists` and `ifColumnExists`
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -141,10 +153,12 @@ UserType apply(KeyspaceMetadata keyspace, UserType userType)
private static final class RenameFields extends AlterTypeStatement
{
private final Map<FieldIdentifier, FieldIdentifier> renamedFields;
+ private final boolean ifFieldExists;
- private RenameFields(String keyspaceName, String typeName, Map<FieldIdentifier, FieldIdentifier> renamedFields)
+ private RenameFields(String keyspaceName, String typeName, Map<FieldIdentifier, FieldIdentifier> renamedFields, boolean ifFieldExists, boolean ifExists)
Review comment:
I would put first `ifExists` then `ifFieldExists`
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -77,23 +77,29 @@
public abstract class AlterTableStatement extends AlterSchemaStatement
{
protected final String tableName;
+ private final boolean ifExists;
- public AlterTableStatement(String keyspaceName, String tableName)
+ public AlterTableStatement(String keyspaceName, String tableName, boolean ifExists)
{
super(keyspaceName);
this.tableName = tableName;
+ this.ifExists = ifExists;
}
public Keyspaces apply(Keyspaces schema) throws UnknownHostException
{
KeyspaceMetadata keyspace = schema.getNullable(keyspaceName);
TableMetadata table = null == keyspace
- ? null
- : keyspace.getTableOrViewNullable(tableName);
+ ? null
+ : keyspace.getTableOrViewNullable(tableName);
Review comment:
You should keep the original formatting.
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -443,8 +470,8 @@ public KeyspaceMetadata apply(KeyspaceMetadata keyspace, TableMetadata table)
validateCanDropCompactStorage();
Set<Flag> flags = table.isCounter()
- ? ImmutableSet.of(Flag.COMPOUND, Flag.COUNTER)
- : ImmutableSet.of(Flag.COMPOUND);
+ ? ImmutableSet.of(Flag.COMPOUND, Flag.COUNTER)
Review comment:
I would keep the original formatting
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -163,18 +169,20 @@ public KeyspaceMetadata apply(KeyspaceMetadata keyspace, TableMetadata table)
}
private final Collection<Column> newColumns;
+ private final boolean ifColumnNotExists;
- private AddColumns(String keyspaceName, String tableName, Collection<Column> newColumns)
+ private AddColumns(String keyspaceName, String tableName, Collection<Column> newColumns, boolean ifColumnNotExists, boolean ifTableExists)
Review comment:
nit: I would put the `ifTableExists` first as from a statement point of view you expect the table first
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -313,23 +332,25 @@ private long getTimestamp()
}
/**
- * ALTER TABLE <table> RENAME <column> TO <column>;
+ * ALTER TABLE [IF EXISTS] <table> RENAME [IF EXISTS] <column> TO <column>;
*/
private static class RenameColumns extends AlterTableStatement
{
private final Map<ColumnIdentifier, ColumnIdentifier> renamedColumns;
+ private final boolean ifColumnsExists;
- private RenameColumns(String keyspaceName, String tableName, Map<ColumnIdentifier, ColumnIdentifier> renamedColumns)
+ private RenameColumns(String keyspaceName, String tableName, Map<ColumnIdentifier, ColumnIdentifier> renamedColumns, boolean ifColumnsExists, boolean ifTableExists)
Review comment:
nit: I would put the ifTableExists first.
--
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