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/01/28 20:40:04 UTC
[GitHub] [cassandra] yifan-c opened a new pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
yifan-c opened a new pull request #883:
URL: https://github.com/apache/cassandra/pull/883
----------------------------------------------------------------
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.
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] smiklosovic closed pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #883:
URL: https://github.com/apache/cassandra/pull/883
--
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] blerer commented on a change in pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #883:
URL: https://github.com/apache/cassandra/pull/883#discussion_r566907931
##########
File path: test/unit/org/apache/cassandra/tools/cqlsh/CqlshTest.java
##########
@@ -44,4 +48,29 @@ public void testKeyspaceRequired()
assertThat(tool.getCleanedStderr(), CoreMatchers.containsStringIgnoringCase("No keyspace has been specified"));
assertEquals(2, tool.getExitCode());
}
+
+ @Test
+ public void testUseUnreservedKeywordAsColumnName()
+ {
+ List<String> names = Arrays.asList("access", "datacenters");
+ for (String name : names)
+ {
+ testUseUnreservedKeywordAsColumnName(name);
+ testUseUnreservedKeywordAsColumnName(name.toUpperCase());
+ }
+ }
Review comment:
nit: I would have put that test into `CreateTest`. I would not think of looking into CqlshTest for a test on unreserved Keywords.
----------------------------------------------------------------
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.
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] blerer commented on a change in pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #883:
URL: https://github.com/apache/cassandra/pull/883#discussion_r567829322
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/CreateTest.java
##########
@@ -688,12 +692,16 @@ public void testCreateTableWithCompression() throws Throwable
@Test
public void testUseUnreservedKeywordAsColumnName()
{
- List<String> names = Arrays.asList("access", "datacenters");
- for (String colName : names)
+ Set<String> allKeywords = Arrays.stream(CqlLexer.class.getDeclaredFields())
+ .filter(f -> f.getName().startsWith("K_"))
+ .map(f -> f.getName().substring(2)) // remove the heading "K_"
+ .collect(Collectors.toSet());
+ Set<String> unreservedKeywords = Sets.difference(allKeywords, ReservedKeywords.reservedSet);
Review comment:
Another approach would be to make `isReserved(String text)` `public` and to use it to filter the stream.
```
Set<String> unreservedKeywords = Arrays.stream(CqlLexer.class.getDeclaredFields())
.filter(f -> f.getName().startsWith("K_"))
.map(f -> f.getName().substring(2)) // remove the heading "K_"
.filter(f -> !ReservedKeywords.isReserved(f))
.collect(Collectors.toSet());
```
Making `isReserved(String text)` `public` allow us to expose less of the class internals but I do not really have a strong opinion on which solution we should use.
##########
File path: src/java/org/apache/cassandra/cql3/ReservedKeywords.java
##########
@@ -23,7 +23,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
-class ReservedKeywords
+public class ReservedKeywords
Review comment:
We should probably make the class `final`.
----------------------------------------------------------------
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.
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] blerer commented on pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
blerer commented on pull request #883:
URL: https://github.com/apache/cassandra/pull/883#issuecomment-770857698
Your new test is a great idea. :-) We often messed up with reserved and unreserved keywords.
I do not believe that changing the access level is an issue.
----------------------------------------------------------------
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.
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] yifan-c edited a comment on pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
yifan-c edited a comment on pull request #883:
URL: https://github.com/apache/cassandra/pull/883#issuecomment-770021253
@blerer thanks for the feedback.
I move the test to `CreateTest` and rewrite using `createTable()` instead of invoking cqlsh.
I also made an optional commit c81be6f that determines all the unreserved keywords and enumerate through to give the best coverage. It helps to prevent the scenario that adds a keyword but forgot to add it to either unreserved or reserved list.
However, the commit opens up the access level to `ReservedKeywords` class. Would it be a concern? Intuitively, I think "no". Or if we want to keep the access level, we can move the test into `ReservedKeywordsTest` (and potentially change the test class name)?
----------------------------------------------------------------
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.
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] yifan-c commented on pull request #883: CASSANDRA-16398: add ACCESS and DATACENTERS to unreserved keywords
Posted by GitBox <gi...@apache.org>.
yifan-c commented on pull request #883:
URL: https://github.com/apache/cassandra/pull/883#issuecomment-770021253
@blerer thanks for the feedback.
I move the test to `CreateTest` and rewrite using `createTable()` instead of invoking cqlsh.
I also made an optional commit c81be6f that determines all the unreserved keywords and enumerate through to give the best coverage. It helps to prevent the scenario that add a keyword but forgot to add it to either unreserved or reserved.
However, the commit opens up the access level to `ReservedKeywords` class. Would it be a concern? Intuitively, I think "no". Or if we want to keep the access level, we can move the test into `ReservedKeywordsTest` (and potentially change the test class name)?
----------------------------------------------------------------
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.
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