adelapena commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1252191112
##########
doc/modules/cassandra/examples/CQL/create_index.cql:
##########
@@ -1,6 +1,7 @@
CREATE INDEX userIndex ON NerdMovies (user);
CREATE INDEX ON Mutants (abilityId);
-CREATE INDEX ON users (keys(favs));
+CREATE INDEX ON users (KEYS(favs));
+CREATE INDEX ON users (age) USING 'sai';
Review Comment:
I think we need to add the doc changes in `cql_singlefile.adoc` too. That
file seems to be an aggregation of the individual files in the same directory.
##########
test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java:
##########
@@ -161,6 +170,37 @@ private static String removeQuotes(String indexName)
return StringUtils.remove(indexName, '\"');
}
+ @Test
+ public void shouldCreateCassandraIndexExplicitly() throws Throwable
+ {
+ createTable("CREATE TABLE %s (userid uuid PRIMARY KEY, firstname text,
lastname text, age int)");
+ createIndex("CREATE INDEX byAge ON %s(age) USING 'legacy'");
Review Comment:
The test doesn't verify that the proper index implementation is selected. I
mean, the test would pass if a SAI index were created. I think we can do that
check with something like:
```java
SecondaryIndexManager indexManager =
getCurrentColumnFamilyStore().indexManager;
Index index = indexManager.getIndexByName("byage");
assertTrue(index instanceof CassandraIndex);
```
##########
test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java:
##########
@@ -161,6 +170,37 @@ private static String removeQuotes(String indexName)
return StringUtils.remove(indexName, '\"');
}
+ @Test
+ public void shouldCreateCassandraIndexExplicitly() throws Throwable
+ {
+ createTable("CREATE TABLE %s (userid uuid PRIMARY KEY, firstname text,
lastname text, age int)");
+ createIndex("CREATE INDEX byAge ON %s(age) USING 'legacy'");
+
+ UUID id1 = UUID.fromString("550e8400-e29b-41d4-a716-446655440000");
+ execute("INSERT INTO %s (userid, firstname, lastname, age) VALUES (?,
'Frodo', 'Baggins', 32)", id1);
+ assertEmpty(execute("SELECT firstname FROM %s WHERE userid = ? AND age
= 33", id1));
+ }
+
+ @Test
+ public void shouldCreateCassandraIndexWhenNotDefault() throws Throwable
+ {
+ DatabaseDescriptor.setDefaultSecondaryIndex(StorageAttachedIndex.NAME);
+
+ try
+ {
+ createTable("CREATE TABLE %s (userid uuid PRIMARY KEY, firstname
text, lastname text, age int)");
+ createIndex("CREATE INDEX byAge ON %s(age) USING 'legacy'");
Review Comment:
As before, we probably should verify that a legacy index is created.
##########
test/unit/org/apache/cassandra/index/sai/cql/CollectionIndexingTest.java:
##########
@@ -220,4 +222,9 @@ private void assertUnsupportedIndexOperator(int
expectedSize, String query, Obje
assertInvalidMessage(StatementRestrictions.REQUIRES_ALLOW_FILTERING_MESSAGE,
query, values);
assertEquals(expectedSize, execute(query + " ALLOW FILTERING").size());
}
+
+ private static String createIndexDDL(String target)
Review Comment:
I think I prefer `createIdexDDL` because we are inside the SAI package. We
have lots of mentions to "index" in that package and IMO it won't be practical
to prefix all of them with "SAI" given the context.
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateIndexStatement.java:
##########
@@ -316,6 +318,23 @@ public CreateIndexStatement prepare(ClientState state)
if (indexName.hasKeyspace() &&
!keyspaceName.equals(indexName.getKeyspace()))
throw ire(KEYSPACE_DOES_NOT_MATCH_INDEX, keyspaceName,
tableName);
+
+ // Set the configured default 2i implementation if one isn't
specified with USING:
+ if (attrs.customClass == null)
+ if (DatabaseDescriptor.getDefaultSecondaryIndexEnabled())
+ attrs.customClass =
DatabaseDescriptor.getDefaultSecondaryIndex();
+ else
+ // However, operators may require an implementation be
specified
+ throw ire(MUST_SPECIFY_INDEX_IMPLEMENTATION);
+
+ // If we explicitly specify the index type "cassandra", we can
just clear the custom class, and the
+ // non-custom 2i creation process will begin. Otherwise, if an
index type has been specified with
+ // USING, make sure the appropriate custom index is created.
+ if (attrs.customClass != null)
Review Comment:
Nit: I'd add braces to the outer if
##########
test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java:
##########
@@ -255,20 +257,66 @@ public void
shouldFailCreateWithInvalidCharactersInColumnName()
public void shouldCreateIndexIfExists()
{
createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)");
+ createIndex("CREATE INDEX IF NOT EXISTS ON %s(val) USING 'sai' ");
+ createIndexAsync("CREATE INDEX IF NOT EXISTS ON %s(val) USING 'sai' ");
- createIndex("CREATE CUSTOM INDEX IF NOT EXISTS ON %s(val) USING
'StorageAttachedIndex' ");
+ assertEquals(1, saiCreationCounter.get());
+ }
+
+ @Test
+ public void shouldCreateIndexCaseInsensitive()
+ {
+ createTable("CREATE TABLE %s (id text PRIMARY KEY, val1 text, val2
text)");
+ createIndex("CREATE INDEX mixed_case_val ON %s(val1) USING 'Sai' ");
+ createIndex("CREATE INDEX upper_case_val ON %s(val2) USING 'SAI' ");
+
+ assertEquals(2, saiCreationCounter.get());
+ }
- createIndexAsync("CREATE CUSTOM INDEX IF NOT EXISTS ON %s(val) USING
'StorageAttachedIndex' ");
+ @Test
+ public void shouldCreateIndexWithClassName()
+ {
+ createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)");
+ createIndex("CREATE INDEX ON %s(val) USING 'StorageAttachedIndex' ");
+ assertEquals(1, saiCreationCounter.get());
+ }
+ @Test
+ public void shouldCreateIndexWithDefault()
+ {
+ DatabaseDescriptor.setDefaultSecondaryIndex(StorageAttachedIndex.NAME);
+ createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)");
+ createIndex("CREATE INDEX ON %s(val)");
assertEquals(1, saiCreationCounter.get());
}
+ @Test
+ public void shouldFailWithDefaultIndexDisabled()
+ {
+ DatabaseDescriptor.setDefaultSecondaryIndex(StorageAttachedIndex.NAME);
+ boolean original =
DatabaseDescriptor.getDefaultSecondaryIndexEnabled();
+
+ try
+ {
+
Review Comment:
Nit: unneeded blank line
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateIndexStatement.java:
##########
@@ -316,6 +318,23 @@ public CreateIndexStatement prepare(ClientState state)
if (indexName.hasKeyspace() &&
!keyspaceName.equals(indexName.getKeyspace()))
throw ire(KEYSPACE_DOES_NOT_MATCH_INDEX, keyspaceName,
tableName);
+
+ // Set the configured default 2i implementation if one isn't
specified with USING:
+ if (attrs.customClass == null)
Review Comment:
Nit: I'd add braces to the outer if
--
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]
