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/25 06:28:46 UTC
[GitHub] [cassandra] djanand opened a new pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
djanand opened a new pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422
#### Add support for IF EXISTS and IF NOT EXISTS in ALTER statements
* add ifexists and ifnotexists for other stmts
* addressing review comments
* review comments:changes to cqlsh with test-cases
* addressing review comments #2
* fixing dtest and misplaced stmt
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828800595
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -205,6 +223,9 @@ UserType apply(KeyspaceMetadata keyspace, UserType userType)
}
private final UTName name;
+ private final boolean ifExists;
Review comment:
hmmm, I had that earlier, but it seems that the consensus from an earlier PR ( that I made on top of 4.0 branch unfortunately) was to convert them to ifExists. The understanding was that the meaning of the variable can be infered from context. Here are the previous review comments.
https://github.com/apache/cassandra/pull/1319#discussion_r754352408
https://github.com/apache/cassandra/pull/1319#discussion_r755154353
https://github.com/apache/cassandra/pull/1319#discussion_r755155148
https://github.com/apache/cassandra/pull/1319#discussion_r754354472
https://github.com/apache/cassandra/pull/1319#discussion_r754355124
https://github.com/apache/cassandra/pull/1319#discussion_r754353396
https://github.com/apache/cassandra/pull/1319#discussion_r754352775
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828815091
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
You want me to create it or do you want to have a go yourself?
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828812337
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,45 @@ 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; }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
Review comment:
Thanks! Yeah you are right! I was referencing existing stmts where something similar was done. Hence choose to take that route.
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828830466
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
Created it here https://issues.apache.org/jira/browse/CASSANDRA-17447.
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828830466
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
Created it here https://issues.apache.org/jira/browse/CASSANDRA-17447 Please make changes if the wording and context isn't clear. Not sure if we follow a specific template when creating tickets.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826775721
##########
File path: src/antlr/Parser.g
##########
@@ -961,28 +969,33 @@ 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;
Review comment:
I think this one is not being used?
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828811759
##########
File path: src/antlr/Parser.g
##########
@@ -961,28 +969,33 @@ 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;
Review comment:
thanks for keeping a close 👀 . That's right, it wasn't being used. Removed and ran tests as well. Looks good
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828824554
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -821,8 +821,7 @@ public void testAlterTableAddColWithIfNotExists() throws Throwable
public void testAlterTableAddExistingColumnWithoutIfExists()
{
createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
- assertAlterTableThrowsException(InvalidRequestException.class,
- String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
+ assertAlterTableThrowsException(InvalidRequestException.class, String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
Review comment:
Sorry for the misunderstanding. I should have gone through the C* code style templates better. Will keep this in mind and review code-styles. Fixed it in the latest commit. Agree it a nit, but I think it's good to follow the correct way of doing it. Appreciate it.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828814336
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -821,8 +821,7 @@ public void testAlterTableAddColWithIfNotExists() throws Throwable
public void testAlterTableAddExistingColumnWithoutIfExists()
{
createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
- assertAlterTableThrowsException(InvalidRequestException.class,
- String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
+ assertAlterTableThrowsException(InvalidRequestException.class, String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
Review comment:
I was referring to the fact we try to avoid long lines. This would probably end up in 3 lines, one for each arg, and the 3 args aligned. Look [here](https://github.com/apache/cassandra/blob/46f617900d1f2061bff38440036b420216634816/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java#L165) i.e. or the C* code style
It's a nit, but just to let you know this is how we format things.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826779274
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,45 @@ 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; }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
Review comment:
Wouldn't it be nicer, instead of polluting the constructor, you had a setter for `ifExists` like you do later on with the exists flags at column level? It would be more consistent and I don't see a need to have that in the ctor? wdyt?
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826859875
##########
File path: test/unit/org/apache/cassandra/cql3/ViewSchemaTest.java
##########
@@ -842,6 +842,14 @@ public void testViewMetadataCQLIncludeAllColumn()
expectedViewSnapshot);
}
+ @Test
+ public void testAlterViewIfExists() throws Throwable
+ {
+ execute("USE " + keyspace());
+ executeNet("USE " + keyspace());
Review comment:
Why twice?
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828814336
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -821,8 +821,7 @@ public void testAlterTableAddColWithIfNotExists() throws Throwable
public void testAlterTableAddExistingColumnWithoutIfExists()
{
createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
- assertAlterTableThrowsException(InvalidRequestException.class,
- String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
+ assertAlterTableThrowsException(InvalidRequestException.class, String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
Review comment:
I was referring to the fact we try to avoid long lines. This would probably end up in 3 lines, one for each arg, and the 3 args aligned. Look [here](https://github.com/apache/cassandra/pull/1422/commits/46f617900d1f2061bff38440036b420216634816#diff-60155f5fd589ec7d5d5cba38462b429d211fa0c70a3c3283972dd6e865c1eebbR189) i.e. or the C* code style
It's a nit, but just to let you know this is how we format things.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826757543
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1443,7 +1443,7 @@ def alter_type_field_completer(ctxt, cass):
( "SUPERUSER" | "NOSUPERUSER" )?
;
-<alterUserStatement> ::= "ALTER" "USER" <username>
+<alterUserStatement> ::= "ALTER" "USER" ("IF" "EXISTS")? <username>
Review comment:
Despite the original intent of the ticket mentioning only `ALTER` if we're going to touch `ALTER USER` maybe we should also do `DROP USER` and friends, wdyt @ekaterinadimitrova2 @blerer ? Otherwise it feels like this is only half way through to me.
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828801534
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
Yes, 😄 an opportunity for another ticket to add _alter MV stmt in cqlsh_. 👍
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826794665
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -205,6 +223,9 @@ UserType apply(KeyspaceMetadata keyspace, UserType userType)
}
private final UTName name;
+ private final boolean ifExists;
Review comment:
I would rename all these `ifExists` to `ifTableExists` for readability throughout the PR.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826859163
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
Mmmm we don't seem to have an alter MV stmnt, weird... Food for another ticket I guess.
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828812752
##########
File path: test/unit/org/apache/cassandra/cql3/ViewSchemaTest.java
##########
@@ -842,6 +842,14 @@ public void testViewMetadataCQLIncludeAllColumn()
expectedViewSnapshot);
}
+ @Test
+ public void testAlterViewIfExists() throws Throwable
+ {
+ execute("USE " + keyspace());
+ executeNet("USE " + keyspace());
Review comment:
thanks for taking time to keep a close eye on this. Removed 848L
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826859163
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1390,21 +1390,21 @@ def idx_ks_idx_name_completer(ctxt, cass):
syntax_rules += r'''
-<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) cf=<columnFamilyName>
+<alterTableStatement> ::= "ALTER" wat=( "COLUMNFAMILY" | "TABLE" ) ("IF" "EXISTS")? cf=<columnFamilyName>
Review comment:
Mmmm we don't seem to have an later MV stmnt, weird... Food for another ticket I guess.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826757543
##########
File path: pylib/cqlshlib/cql3handling.py
##########
@@ -1443,7 +1443,7 @@ def alter_type_field_completer(ctxt, cass):
( "SUPERUSER" | "NOSUPERUSER" )?
;
-<alterUserStatement> ::= "ALTER" "USER" <username>
+<alterUserStatement> ::= "ALTER" "USER" ("IF" "EXISTS")? <username>
Review comment:
Despite the original intent of the ticket mentioning only `ALTER` if we're going to touch `ALTER USER` maybe we should also do `DROP USER` and friends, wdyt @ekaterinadimitrova2 @blerer ? Otherwise it feels like this is only half way through to me.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r826792634
##########
File path: src/antlr/Parser.g
##########
@@ -915,37 +915,45 @@ 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; }
+ : K_ALTER K_COLUMNFAMILY (K_IF K_EXISTS { ifExists = true; } )?
+ cf=columnFamilyName { $stmt = new AlterTableStatement.Raw(cf, ifExists); }
Review comment:
Oh I see you probably did that to keep it consistent with the already existing stmnts that do use the ctor.... mmmm I don't have a string opinion up to you. Feel free to ignore.
--
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
[GitHub] [cassandra] djanand commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
djanand commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828812886
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -780,4 +781,110 @@ public void testAlterByAddingColumnToCompactTableShouldFail() throws Throwable
assertInvalidMessage("Cannot add new column to a COMPACT STORAGE table",
"ALTER TABLE %s ADD column1 text");
}
+
+ @Test
+ public void testAlterTableWithoutCreateTableOrIfExistsClause()
+ {
+ String tbl1 = KEYSPACE + "." + createTableName();
+ assertAlterTableThrowsException(InvalidRequestException.class, String.format("Table '%s' doesn't exist", tbl1),
+ "ALTER TABLE %s ADD myCollection list<text>;");
+ }
+
+ @Test
+ public void testAlterTableWithoutCreateTableWithIfExists() throws Throwable
+ {
+ String tbl1 = KEYSPACE + "." + createTableName();
+ assertNull(execute(String.format("ALTER TABLE IF EXISTS %s ADD myCollection list<text>;", tbl1)));
+ }
+
+ @Test
+ public void testAlterTableWithIfExists() throws Throwable
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ alterTable("ALTER TABLE IF EXISTS %s ADD myCollection list<text>;");
+ execute("INSERT INTO %s (a, b, myCollection) VALUES (1, 2, ['first element']);");
+
+ assertRows(execute("SELECT * FROM %s;"), row(1, 2, list("first element")));
+ }
+
+ @Test
+ public void testAlterTableAddColWithIfNotExists() throws Throwable
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ alterTable("ALTER TABLE %s ADD IF NOT EXISTS a int;");
+ execute("INSERT INTO %s (a, b) VALUES (1, 2);");
+
+ assertRows(execute("SELECT * FROM %s;"), row(1, 2));
+ }
+
+ @Test
+ public void testAlterTableAddExistingColumnWithoutIfExists()
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ assertAlterTableThrowsException(InvalidRequestException.class,
+ String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
Review comment:
Addressed the formatting in latest commit
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: CASSANDRA-16916 modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r828811932
##########
File path: src/java/org/apache/cassandra/cql3/statements/schema/AlterTypeStatement.java
##########
@@ -205,6 +223,9 @@ UserType apply(KeyspaceMetadata keyspace, UserType userType)
}
private final UTName name;
+ private final boolean ifExists;
Review comment:
Oh Benjamin and myself always disagree on the small readability nits lol!
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1422: modify alter statements with IF EXISTS and IF NOT EXISTS
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1422:
URL: https://github.com/apache/cassandra/pull/1422#discussion_r827707144
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java
##########
@@ -780,4 +781,110 @@ public void testAlterByAddingColumnToCompactTableShouldFail() throws Throwable
assertInvalidMessage("Cannot add new column to a COMPACT STORAGE table",
"ALTER TABLE %s ADD column1 text");
}
+
+ @Test
+ public void testAlterTableWithoutCreateTableOrIfExistsClause()
+ {
+ String tbl1 = KEYSPACE + "." + createTableName();
+ assertAlterTableThrowsException(InvalidRequestException.class, String.format("Table '%s' doesn't exist", tbl1),
+ "ALTER TABLE %s ADD myCollection list<text>;");
+ }
+
+ @Test
+ public void testAlterTableWithoutCreateTableWithIfExists() throws Throwable
+ {
+ String tbl1 = KEYSPACE + "." + createTableName();
+ assertNull(execute(String.format("ALTER TABLE IF EXISTS %s ADD myCollection list<text>;", tbl1)));
+ }
+
+ @Test
+ public void testAlterTableWithIfExists() throws Throwable
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ alterTable("ALTER TABLE IF EXISTS %s ADD myCollection list<text>;");
+ execute("INSERT INTO %s (a, b, myCollection) VALUES (1, 2, ['first element']);");
+
+ assertRows(execute("SELECT * FROM %s;"), row(1, 2, list("first element")));
+ }
+
+ @Test
+ public void testAlterTableAddColWithIfNotExists() throws Throwable
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ alterTable("ALTER TABLE %s ADD IF NOT EXISTS a int;");
+ execute("INSERT INTO %s (a, b) VALUES (1, 2);");
+
+ assertRows(execute("SELECT * FROM %s;"), row(1, 2));
+ }
+
+ @Test
+ public void testAlterTableAddExistingColumnWithoutIfExists()
+ {
+ createTable("CREATE TABLE %s (a int, b int, PRIMARY KEY (a, b)); ");
+ assertAlterTableThrowsException(InvalidRequestException.class,
+ String.format("Column with name '%s' already exists", "a"), "ALTER TABLE IF EXISTS %s ADD a int");
Review comment:
Formatting
--
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