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 2021/11/23 14:15:41 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_r754350273
##########
File path: src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
##########
@@ -62,7 +65,10 @@ 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))
+ {
+ if (ifRoleExists) return;
throw new InvalidRequestException(String.format("%s doesn't exist", role.getRoleName()));
Review comment:
You can use instead:
`RequestValidations.checkTrue(ifRoleExists, "Role %s doesn't exist", role.getRoleName()); `
##########
File path: src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
##########
@@ -34,17 +35,19 @@
private final RoleResource role;
private final RoleOptions opts;
final DCPermissions dcPermissions;
+ private final boolean ifRoleExists;
Review comment:
You can simply use `ifExists` the rest can be deducted from the context
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,48 @@ dropTriggerStatement returns [DropTriggerStatement.Raw stmt]
;
/**
- * ALTER KEYSPACE <KS> WITH <property> = <value>;
+ * ALTER KEYSPACE [IF EXISTS] <KS> WITH <property> = <value>;
*/
alterKeyspaceStatement returns [AlterKeyspaceStatement.Raw stmt]
- @init { KeyspaceAttributes attrs = new KeyspaceAttributes(); }
- : K_ALTER K_KEYSPACE ks=keyspaceName
- K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs); }
+ @init {
+ KeyspaceAttributes attrs = new KeyspaceAttributes();
+ boolean ifExists = false;
+ }
+ : K_ALTER K_KEYSPACE (K_IF K_EXISTS { ifExists = true; } )? ks=keyspaceName
+ K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs, ifExists); }
;
/**
* ALTER TABLE <table> ALTER <column> TYPE <newtype>;
- * ALTER TABLE <table> ADD <column> <newtype>; | ALTER TABLE <table> ADD (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
- * ALTER TABLE <table> DROP <column>; | ALTER TABLE <table> DROP ( <column>,<column1>.....<column n>)
- * ALTER TABLE <table> RENAME <column> TO <column>;
- * ALTER TABLE <table> WITH <property> = <value>;
+ * ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] <column> <newtype>; | ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
+ * ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] <column>; | ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] ( <column>,<column1>.....<column n>)
+ * ALTER TABLE [IF EXISTS] <table> RENAME [IF EXISTS] <column> TO <column>;
+ * ALTER TABLE [IF EXISTS] <table> WITH <property> = <value>;
*/
alterTableStatement returns [AlterTableStatement.Raw stmt]
- : K_ALTER K_COLUMNFAMILY cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf); }
+ @init {
+ boolean ifExists = false;
+ boolean ifNotExists = false;
+ }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
(
K_ALTER id=cident K_TYPE v=comparatorType { $stmt.alter(id, v); }
- | K_ADD ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b); }
- | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1); }
- ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn); } )* ')') )
+ | K_ADD ( K_IF K_NOT K_EXISTS { ifNotExists = true; } )?
+ ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b, ifNotExists); }
+ | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1, ifNotExists); }
+ ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn, ifNotExists); } )* ')') )
Review comment:
Storing adding the `ifNotExists` with each column duplicate the information and make the rest of the patch much complex than required. The approach I would suggest is to add a new `ifColumnNotExists(boolean)` method to `AlterTableStatement.Raw` that can be us after the K_NOT K_EXISTS. Columns can then be added separately.
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -93,7 +94,12 @@ public Keyspaces apply(Keyspaces schema) throws UnknownHostException
: keyspace.getTableOrViewNullable(tableName);
if (null == table)
+ {
+ if (ifTableExists)
+ return schema;
Review comment:
```
if (!ifExists)
throw ire("Table '%s.%s' doesn't exist", keyspaceName, tableName);
```
##########
File path: src/antlr/Parser.g
##########
@@ -961,29 +972,34 @@ isStaticColumn returns [boolean isStaticColumn]
alterMaterializedViewStatement returns [AlterViewStatement.Raw stmt]
@init {
TableAttributes attrs = new TableAttributes();
+ boolean ifExists = false;
}
- : K_ALTER K_MATERIALIZED K_VIEW name=columnFamilyName
+ : K_ALTER K_MATERIALIZED K_VIEW (K_IF K_EXISTS { ifExists = true; } )? name=columnFamilyName
K_WITH properties[attrs]
{
- $stmt = new AlterViewStatement.Raw(name, attrs);
+ $stmt = new AlterViewStatement.Raw(name, attrs, ifExists);
}
;
/**
- * ALTER TYPE <name> ALTER <field> TYPE <newtype>;
- * ALTER TYPE <name> ADD <field> <newtype>;
- * ALTER TYPE <name> RENAME <field> TO <newtype> AND ...;
+ * ALTER TYPE [IF EXISTS] <name> ALTER <field> TYPE <newtype>;
+ * ALTER TYPE [IF EXISTS] <name> ADD [IF NOT EXISTS]<field> <newtype>;
+ * ALTER TYPE [IF EXISTS] <name> RENAME [IF EXISTS] <field> TO <newtype> AND ...;
*/
alterTypeStatement returns [AlterTypeStatement.Raw stmt]
- : K_ALTER K_TYPE name=userTypeName { $stmt = new AlterTypeStatement.Raw(name); }
+ @init {
+ boolean ifExists = false;
+ boolean ifNotExists = false;
+ }
+ : K_ALTER K_TYPE (K_IF K_EXISTS { ifExists = true; } )? name=userTypeName { $stmt = new AlterTypeStatement.Raw(name, ifExists); }
(
K_ALTER f=fident K_TYPE v=comparatorType { $stmt.alter(f, v); }
- | K_ADD f=fident v=comparatorType { $stmt.add(f, v); }
+ | K_ADD (K_IF K_NOT K_EXISTS { ifNotExists = true; } )? f=fident v=comparatorType { $stmt.add(f, v, ifNotExists); }
- | K_RENAME f1=fident K_TO toF1=fident { $stmt.rename(f1, toF1); }
- ( K_AND fn=fident K_TO toFn=fident { $stmt.rename(fn, toFn); } )*
+ | K_RENAME (K_IF K_EXISTS { ifExists = true;} )? f1=fident K_TO toF1=fident { $stmt.rename(f1, toF1, ifExists); }
+ ( K_AND fn=fident K_TO toFn=fident { $stmt.rename(fn, toFn, ifExists); } )*
Review comment:
same suggestions that for ALTER TABLE
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,48 @@ dropTriggerStatement returns [DropTriggerStatement.Raw stmt]
;
/**
- * ALTER KEYSPACE <KS> WITH <property> = <value>;
+ * ALTER KEYSPACE [IF EXISTS] <KS> WITH <property> = <value>;
*/
alterKeyspaceStatement returns [AlterKeyspaceStatement.Raw stmt]
- @init { KeyspaceAttributes attrs = new KeyspaceAttributes(); }
- : K_ALTER K_KEYSPACE ks=keyspaceName
- K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs); }
+ @init {
+ KeyspaceAttributes attrs = new KeyspaceAttributes();
+ boolean ifExists = false;
+ }
+ : K_ALTER K_KEYSPACE (K_IF K_EXISTS { ifExists = true; } )? ks=keyspaceName
+ K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs, ifExists); }
;
/**
* ALTER TABLE <table> ALTER <column> TYPE <newtype>;
- * ALTER TABLE <table> ADD <column> <newtype>; | ALTER TABLE <table> ADD (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
- * ALTER TABLE <table> DROP <column>; | ALTER TABLE <table> DROP ( <column>,<column1>.....<column n>)
- * ALTER TABLE <table> RENAME <column> TO <column>;
- * ALTER TABLE <table> WITH <property> = <value>;
+ * ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] <column> <newtype>; | ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
+ * ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] <column>; | ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] ( <column>,<column1>.....<column n>)
+ * ALTER TABLE [IF EXISTS] <table> RENAME [IF EXISTS] <column> TO <column>;
+ * ALTER TABLE [IF EXISTS] <table> WITH <property> = <value>;
*/
alterTableStatement returns [AlterTableStatement.Raw stmt]
- : K_ALTER K_COLUMNFAMILY cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf); }
+ @init {
+ boolean ifExists = false;
+ boolean ifNotExists = false;
+ }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
(
K_ALTER id=cident K_TYPE v=comparatorType { $stmt.alter(id, v); }
- | K_ADD ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b); }
- | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1); }
- ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn); } )* ')') )
+ | K_ADD ( K_IF K_NOT K_EXISTS { ifNotExists = true; } )?
+ ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b, ifNotExists); }
+ | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1, ifNotExists); }
+ ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn, ifNotExists); } )* ')') )
- | K_DROP ( id=ident { $stmt.drop(id); }
- | ('(' id1=ident { $stmt.drop(id1); }
- ( ',' idn=ident { $stmt.drop(idn); } )* ')') )
+ | K_DROP ( K_IF K_EXISTS { ifExists = true;} )?
+ ( id=ident { $stmt.drop(id, ifExists); }
+ | ('(' id1=ident { $stmt.drop(id1, ifExists); }
+ ( ',' idn=ident { $stmt.drop(idn, ifExists); } )* ')') )
Review comment:
I would suggest a similar approach that what I suggested for the ADD columns
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java
##########
@@ -66,7 +68,10 @@ public Keyspaces apply(Keyspaces schema)
KeyspaceMetadata keyspace = schema.getNullable(keyspaceName);
if (null == keyspace)
+ {
+ if(ifKeyspaceExists) return schema;
Review comment:
I would use:
```
if (!ifExists)
throw ire("Keyspace '%s' doesn't exist", keyspaceName);
```
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java
##########
@@ -77,11 +76,13 @@
public abstract class AlterTableStatement extends AlterSchemaStatement
{
protected final String tableName;
+ private final boolean ifTableExists;
Review comment:
ifTableExists -> ifExists
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -49,10 +49,12 @@
public abstract class AlterTypeStatement extends AlterSchemaStatement
{
protected final String typeName;
+ protected final boolean ifTypeExists;
- public AlterTypeStatement(String keyspaceName, String typeName)
+ public AlterTypeStatement(String keyspaceName, String typeName, boolean ifTypeExists)
{
super(keyspaceName);
+ this.ifTypeExists = ifTypeExists;
Review comment:
ifTypeExists -> ifExists
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,48 @@ dropTriggerStatement returns [DropTriggerStatement.Raw stmt]
;
/**
- * ALTER KEYSPACE <KS> WITH <property> = <value>;
+ * ALTER KEYSPACE [IF EXISTS] <KS> WITH <property> = <value>;
*/
alterKeyspaceStatement returns [AlterKeyspaceStatement.Raw stmt]
- @init { KeyspaceAttributes attrs = new KeyspaceAttributes(); }
- : K_ALTER K_KEYSPACE ks=keyspaceName
- K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs); }
+ @init {
+ KeyspaceAttributes attrs = new KeyspaceAttributes();
+ boolean ifExists = false;
+ }
+ : K_ALTER K_KEYSPACE (K_IF K_EXISTS { ifExists = true; } )? ks=keyspaceName
+ K_WITH properties[attrs] { $stmt = new AlterKeyspaceStatement.Raw(ks, attrs, ifExists); }
;
/**
* ALTER TABLE <table> ALTER <column> TYPE <newtype>;
- * ALTER TABLE <table> ADD <column> <newtype>; | ALTER TABLE <table> ADD (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
- * ALTER TABLE <table> DROP <column>; | ALTER TABLE <table> DROP ( <column>,<column1>.....<column n>)
- * ALTER TABLE <table> RENAME <column> TO <column>;
- * ALTER TABLE <table> WITH <property> = <value>;
+ * ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] <column> <newtype>; | ALTER TABLE [IF EXISTS] <table> ADD [IF NOT EXISTS] (<column> <newtype>,<column1> <newtype1>..... <column n> <newtype n>)
+ * ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] <column>; | ALTER TABLE [IF EXISTS] <table> DROP [IF EXISTS] ( <column>,<column1>.....<column n>)
+ * ALTER TABLE [IF EXISTS] <table> RENAME [IF EXISTS] <column> TO <column>;
+ * ALTER TABLE [IF EXISTS] <table> WITH <property> = <value>;
*/
alterTableStatement returns [AlterTableStatement.Raw stmt]
- : K_ALTER K_COLUMNFAMILY cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf); }
+ @init {
+ boolean ifExists = false;
+ boolean ifNotExists = false;
+ }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
(
K_ALTER id=cident K_TYPE v=comparatorType { $stmt.alter(id, v); }
- | K_ADD ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b); }
- | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1); }
- ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn); } )* ')') )
+ | K_ADD ( K_IF K_NOT K_EXISTS { ifNotExists = true; } )?
+ ( id=ident v=comparatorType b=isStaticColumn { $stmt.add(id, v, b, ifNotExists); }
+ | ('(' id1=ident v1=comparatorType b1=isStaticColumn { $stmt.add(id1, v1, b1, ifNotExists); }
+ ( ',' idn=ident vn=comparatorType bn=isStaticColumn { $stmt.add(idn, vn, bn, ifNotExists); } )* ')') )
- | K_DROP ( id=ident { $stmt.drop(id); }
- | ('(' id1=ident { $stmt.drop(id1); }
- ( ',' idn=ident { $stmt.drop(idn); } )* ')') )
+ | K_DROP ( K_IF K_EXISTS { ifExists = true;} )?
+ ( id=ident { $stmt.drop(id, ifExists); }
+ | ('(' id1=ident { $stmt.drop(id1, ifExists); }
+ ( ',' idn=ident { $stmt.drop(idn, ifExists); } )* ')') )
( K_USING K_TIMESTAMP t=INTEGER { $stmt.timestamp(Long.parseLong(Constants.Literal.integer($t.text).getText())); } )?
- | K_RENAME id1=ident K_TO toId1=ident { $stmt.rename(id1, toId1); }
- ( K_AND idn=ident K_TO toIdn=ident { $stmt.rename(idn, toIdn); } )*
+ | K_RENAME ( K_IF K_EXISTS { ifExists = true;} )?
+ ( id1=ident K_TO toId1=ident { $stmt.rename(id1, toId1, ifExists); }
+ ( K_AND idn=ident K_TO toIdn=ident { $stmt.rename(idn, toIdn, ifExists); } )* )
Review comment:
I would suggest a similar approach that what I suggested for the ADD columns
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java
##########
@@ -53,11 +53,13 @@
private final HashSet<String> clientWarnings = new HashSet<>();
private final KeyspaceAttributes attrs;
+ private final boolean ifKeyspaceExists;
Review comment:
ifKeyspaceExists -> ifExists
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -75,7 +77,10 @@ public Keyspaces apply(Keyspaces schema)
: keyspace.types.getNullable(bytes(typeName));
if (null == type)
+ {
+ if(ifTypeExists) return schema;
throw ire("Type %s.%s doesn't exist", keyspaceName, typeName);
Review comment:
```
if (!ifExists)
throw ire("Type %s.%s doesn't exist", keyspaceName, typeName);
```
--
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