Maxwell-Guo commented on code in PR #2433: URL: https://github.com/apache/cassandra/pull/2433#discussion_r1241095731
########## conf/cassandra.yaml: ########## @@ -1956,6 +1956,15 @@ drop_compact_storage_enabled: false # if zero_ttl_on_twcs_enabled is set to false, this property is irrelevant as such statements will fail. #zero_ttl_on_twcs_warned: true +# The default secondary index implementation when CREATE INDEX does not specify one via USING. +# ex. "cassandra" - (default) traditional secondary index, implemented as a hidden table attached to the primary table +# ex. "sai" - storage-attched index, implemented via optimized SSTable/Memtable-attached indexes Review Comment: as for native secondary index ,we may use "cassandra" or some other keyword to represent the default index, then What about indexes other than sai? like sasi (if sasi is not fully removed when sai is released) and some plugin index using CUSTOM keyword like what I have said [lucene-index](https://github.com/Stratio/cassandra-lucene-index) (though we know that this plugin is not update now, but what If someone what to add a plugin index by himself ? as for version before this patch ,he may use the interface defined by CUSTOM , but it seems this patch do not support this feature if someone want to develop his own index-plugin through CUSTOM or there is no description for this kind of index) ########## test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java: ########## @@ -170,7 +174,7 @@ public void shouldFailCreationUsingMode() { createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)"); - assertThatThrownBy(() -> executeNet("CREATE CUSTOM INDEX ON %s(val) USING " + + assertThatThrownBy(() -> executeNet("CREATE INDEX ON %s(val) USING " + Review Comment: so StorageAttachedIndex and sai are both valid, so the document should be update, I think. ########## 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) + if (!attrs.isCustom && attrs.customClass.equalsIgnoreCase(CassandraIndex.NAME)) Review Comment: Wouldn't it be more appropriate for this equation to become `!attrs.isCustom && attrs.customClass.equalsIgnoreCase(DatabaseDescriptor.getDefaultSecondaryIndex()) ` ？ ########## src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java: ########## @@ -94,6 +94,8 @@ public class StorageAttachedIndex implements Index { + public static final String NAME = "sai"; Review Comment: But IndexMetada seems to use the name to build indexNameAliases .... ########## test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java: ########## @@ -70,6 +72,12 @@ public class SecondaryIndexTest extends CQLTester { public static final int TOO_BIG = 1024 * 65; + + @BeforeClass + public static void setDefaultSecondaryIndex() + { + DatabaseDescriptor.setDefaultSecondaryIndex(CassandraIndex.NAME); Review Comment: this test do not cover that when we do not set defaultSecondaryIndex. ########## 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 rename createIndexDDL to createSAIIndexDDL is more expressive ########## test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java: ########## @@ -170,7 +174,7 @@ public void shouldFailCreationUsingMode() { createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)"); - assertThatThrownBy(() -> executeNet("CREATE CUSTOM INDEX ON %s(val) USING " + + assertThatThrownBy(() -> executeNet("CREATE INDEX ON %s(val) USING " + Review Comment: Besides I think the purpose of your test should be to test "USING 'sai' " ? ########## test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java: ########## @@ -285,11 +335,10 @@ public void shouldCreateIndexOnReversedType() throws Throwable } @Test - public void shouldCreateIndexWithAlias() + public void shouldCreateIndexWithFullClassName() { createTable("CREATE TABLE %s (id text PRIMARY KEY, val text)"); - - createIndex("CREATE CUSTOM INDEX ON %s(val) USING 'StorageAttachedIndex'"); + createIndex("CREATE CUSTOM INDEX ON %s(val) USING 'org.apache.cassandra.index.sai.StorageAttachedIndex'"); Review Comment: USING org.apache.cassandra.index.sai.StorageAttachedIndex also passed the test ,So the user document should be updated. ########## src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java: ########## @@ -94,6 +94,8 @@ public class StorageAttachedIndex implements Index { + public static final String NAME = "sai"; Review Comment: what about add a new method named getIndexName() in the interface java Index.java , and every implementation index implement the method and return the real name ? ########## src/java/org/apache/cassandra/schema/IndexMetadata.java: ########## @@ -67,7 +67,8 @@ public final class IndexMetadata static { - indexNameAliases.put(StorageAttachedIndex.class.getSimpleName(), StorageAttachedIndex.class.getCanonicalName()); + indexNameAliases.put(StorageAttachedIndex.NAME, StorageAttachedIndex.class.getCanonicalName()); Review Comment: should we change keyword from "sai" to "StorageAttachedIndex" ? as I think StorageAttachedIndex is more expressive than sai to express the situation of indexing clearly. Sorry, I may be missing some final discussion process for the content of DISCUSS, so I will finally express the idea of replacing the sai keyword. ########## test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java: ########## @@ -303,7 +352,7 @@ public void shouldCreateSASI() throws Throwable { createTable(CREATE_TABLE_TEMPLATE); - createIndex("CREATE CUSTOM INDEX ON %s(v1) USING 'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode': 'CONTAINS',\n" + + createIndex("CREATE INDEX ON %s(v1) USING 'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode': 'CONTAINS',\n" + Review Comment: as I have comment [does sasi will fully removed after sai release](https://issues.apache.org/jira/browse/CASSANDRA-18615) , so it seems SASI is supported too, but as a new user , When I saw the description in this [yaml file](https://github.com/apache/cassandra/pull/2433/files#diff-77707d0908c31940828b6425dcb09a7409827db99b48c371f71c63294dfe1562) , I actually didn't know what the usage of native 2i, sai, sasi, and custom index plugin that could be used before was changed after the update of this patch. So we may need more detailed documentation and add some CHANGE.txt that can clearly tell users about the changes, and how to configure the old functions on the new version。 -- 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]
