You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maedhroz (via GitHub)" <gi...@apache.org> on 2023/06/21 22:30:09 UTC
[GitHub] [cassandra] maedhroz opened a new pull request, #2433: Add support for USING and index creation by name with CREATE INDEX
maedhroz opened a new pull request, #2433:
URL: https://github.com/apache/cassandra/pull/2433
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-18615
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243063351
##########
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:
Thank you for explaining @maedhroz , so what about add a corresponding description to this document? This way, users may understand that the old usage(CREATE CUSTOM INDEX) is still possible.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243116212
##########
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:
There is no change. SAI hasn't been released yet. There is no example of an SAI index being created w/ that syntax in the wild.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1240450683
##########
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())
Review Comment:
The usage here is why I didn't end up making this a guardrail.
--
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] adelapena commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1266540442
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateIndexStatement.java:
##########
@@ -316,6 +318,27 @@ 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
Review Comment:
```suggestion
// If we explicitly specify the index type "legacy_local_table", we can just clear the custom class, and the
```
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242418837
##########
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:
It's an SAI test, but perhaps.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243067042
##########
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:
As far as I am concerned, I have participated in a little discussion with the previous DISSCUSS and had a look at this patch once before, so I have a rough idea of which usage is feasible. For first-time users, the process may not be as smooth.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1265875295
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
@adelapena chimed in with "legacy_local_table", which I like a lot. I'm thinking about proceeding w/ that.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1244625542
##########
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:
1. `CREATE CUSTOM INDEX ON %s(VALUES(value)) USING 'StorageAttachedIndex'" `
2. `createIndexDDL(String target)`
1 is more intuitive than 2, as I know I am creating SAI when I see StorageAttachedIndex, but I have no idea about what kind of index I am going to create when see "createIndexDDL".
Of course, you don’t need to change it if you do not want to, because this is just a very small point, which is insignificant
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242481362
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
Posted an informal poll in ASF Slack's #cassandra-sai chat
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
Posted an informal poll in ASF Slack's `#cassandra-sai` chat
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242440620
##########
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:
There were two options to make sure this test works as intended independent of what the YAML says. One is this, just making sure the legacy 2i is the default, and none of the tests themselves need to change their index creation DDL. The other would be changing all the index creation DDL. I chose the former to keep the diff smaller.
`shouldCreateCassandraIndexExplicitly` though...that could change, or a new test be added to check explicit legacy 2i creation when it isn't the default.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1241015723
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
This is the point I most want to comment on this patch~~~ Because cassandra vs SAI cannot be aligned conceptually in my mind. Even native2i is better than cassandra
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1241028770
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
I’d really like to call it “legacy” to reflect my belief that we will eventually treat SAI as the default 2i implementation.
--
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] maedhroz closed pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
URL: https://github.com/apache/cassandra/pull/2433
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1241030219
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
legacy is also one of the potential options I think
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242403756
##########
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:
There's nothing stopping anyone from developing custom 2i implementations as they always have and creating them using the `CREATE CUSTOM INDEX` DDL. We could always build a service provider framework or something here to register the index name aliases for custom 2i implementations and support them in CREATE INDEX...but not in this patch.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243065356
##########
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:
Yes, if we suggest users to use a certain method, at least a document description or some warning is required. In my experience, if not, users either cannot use it or use it blindly.
In the end, this experience was terrible
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243142803
##########
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:
Pushed. Even this is just to illustrate basic supported usage. The SAI `README.md` in `o.a.c.index.sai` is probably a better in-tree overview of what SAI actually is.
--
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] adelapena commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
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: 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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242447481
##########
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:
Added a new case... `shouldCreateCassandraIndexWhenNotDefault`
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243115861
##########
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:
Once we settle on a name for the legacy 2i and update that in the YAML and `Config`, it would be useful to update `CQL.textile` and `indexes.adoc` (which is the source for the OSS website docs) to reflect the recommended usage. I'll have another commit that includes that shortly...
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1244628761
##########
doc/cql3/CQL.textile:
##########
@@ -511,14 +511,24 @@ __Sample:__
bc(sample).
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';
CREATE CUSTOM INDEX ON users (email) USING 'path.to.the.IndexClass';
CREATE CUSTOM INDEX ON users (email) USING 'path.to.the.IndexClass' WITH OPTIONS = {'storage': '/mnt/ssd/indexes/'};
The @CREATE INDEX@ statement is used to create a new (automatic) secondary index for a given (existing) column in a given table. A name for the index itself can be specified before the @ON@ keyword, if desired. If data already exists for the column, it will be indexed asynchronously. After the index is created, new data for the column is indexed automatically at insertion time.
Attempting to create an already existing index will return an error unless the @IF NOT EXISTS@ option is used. If it is used, the statement will be a no-op if the index already exists.
+h4(#usingIndex). Index Types
+
+The @USING@ keyword optionally specifies an index type. There are two built-in types:
+
+* legacy - (default) legacy secondary index, implemented as a hidden table
+* sai - "storage-attched" index, implemented via optimized SSTable/Memtable-attached indexes
+
+To create a custom index, a fully qualified class name must be specified.
Review Comment:
thanks, I think this is enough.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243115402
##########
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:
At least, "this usage will not affect previous usage" is needed though you said you don't want to document it ?
Update the document do not means we encourage people to use it, just let them know more clear about the change. Then they can know how to use it clearly.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243115402
##########
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:
At least, "this usage will not affect previous usage" is needed though you said you don't want to document it ?
Update the document do not means we encourage people to use it, just let them know more clear about the change.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242431392
##########
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:
We don't want users to use this for SAI. It's only left here for legacy support. I wouldn't object to a client warning of some kind though.
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
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: 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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242416472
##########
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:
You can also use "StorageAttachedIndex" (see the line below).
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242425128
##########
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:
"StorageAttachedIndex" support could be removed entirely, but it doesn't hurt to keep it. I'm kind of fine either way. CC @adelapena @mike-tr-adamson
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243125725
##########
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:
ok~~thanks
--
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] Maxwell-Guo commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243125725
##########
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:
ok~~
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1240451206
##########
src/java/org/apache/cassandra/index/internal/CassandraIndex.java:
##########
@@ -68,6 +68,8 @@
*/
public abstract class CassandraIndex implements Index
{
+ public static final String NAME = "cassandra";
Review Comment:
The name "cassandra" here isn't perfect I suppose, but it's trying to a.) align w/ the class name at least somewhat and b.) connote that this is the "traditional cassandra hidden table" index.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242427124
##########
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:
The purpose of the test is to make sure we reject invalid `WITH OPTIONS` content, but I'll probably change it to `USING 'sai'`. I just missed 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] maedhroz commented on pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#issuecomment-1640890480
Committed as https://github.com/apache/cassandra/commit/4b43c75b00703c5863228331326ecc95d3fe1605
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1267189704
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateIndexStatement.java:
##########
@@ -316,6 +318,27 @@ 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
Review Comment:
Good catch! I'll fix on 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] maedhroz commented on pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#issuecomment-1605037450
https://app.circleci.com/pipelines/github/maedhroz/cassandra?branch=CASSANDRA-18615
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242452883
##########
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:
The scope of this patch isn't really meant to extend to the `CREATE CUSTOM INDEX` DDL or its documentation. Whether we want to deprecate it is another question...
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242456006
##########
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:
See https://github.com/apache/cassandra/pull/2433#discussion_r1242403756
> We could always build a service provider framework or something here to register the index name aliases for custom 2i implementations and support them in CREATE INDEX...but not in this patch.
The other complication here is that `CassandraIndex` is an abstract class. It's subclasses were never meant to be exposed to the user, which makes the SPI thing a little more sticky.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1243107657
##########
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:
My only concern w/ this patch was not to break the `CREATE CUSTOM INDEX` DDL that *happens* to still work as a result of how support for custom indexes was implemented. Even if it's possible, we don't want to document it or encourage people to use it. To be honest, I think the `CREATE CUSTOM INDEX` syntax was probably a mistake.
--
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] maedhroz commented on a diff in pull request #2433: CASSANDRA-18615 Add support for USING and index creation by name with CREATE INDEX
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2433:
URL: https://github.com/apache/cassandra/pull/2433#discussion_r1242412630
##########
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:
No. if the default is "sai", we need the value of `customClass` (which is "sai") to be retained, not discarded. This is to specifically reflect the fact that the "cassandra" or "legacy" type doesn't need a `customClass`.
--
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