adelapena commented on a change in pull request #1159:
URL: https://github.com/apache/cassandra/pull/1159#discussion_r703402650
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASICQLTest.java
##########
@@ -348,4 +351,21 @@ public void testIntCompare() throws Throwable
}
}
}
+
+ @Test
+ public void testInOperator() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int primary key, v int);");
+
+ createIndex(String.format("CREATE CUSTOM INDEX ON %%s (v) USING
'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode':
'CONTAINS'};"));
Review comment:
Nit: we don't need `String.format` here:
```suggestion
createIndex("CREATE CUSTOM INDEX ON %s (v) USING
'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode':
'CONTAINS'}");
```
Also, I think that the `contains` mode is relevant for the test so we might
slightly simplify to:
```suggestion
createIndex("CREATE CUSTOM INDEX ON %s (v) USING
'org.apache.cassandra.index.sasi.SASIIndex'");
```
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASICQLTest.java
##########
@@ -348,4 +351,21 @@ public void testIntCompare() throws Throwable
}
}
}
+
+ @Test
+ public void testInOperator() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int primary key, v int);");
+
+ createIndex(String.format("CREATE CUSTOM INDEX ON %%s (v) USING
'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode':
'CONTAINS'};"));
+
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 0, 100);
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 1, 200);
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 2, 300);
Review comment:
I think we don't need the insertions if we are only going to verify
query validation.
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASICQLTest.java
##########
@@ -348,4 +351,21 @@ public void testIntCompare() throws Throwable
}
}
}
+
+ @Test
+ public void testInOperator() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int primary key, v int);");
+
+ createIndex(String.format("CREATE CUSTOM INDEX ON %%s (v) USING
'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = {'mode':
'CONTAINS'};"));
+
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 0, 100);
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 1, 200);
+ execute("INSERT INTO %s (pk, v) VALUES (?, ?);", 2, 300);
+
+ assertInvalidThrowMessage(Optional.of(ProtocolVersion.CURRENT),
+
StatementRestrictions.REQUIRES_ALLOW_FILTERING_MESSAGE,
+ InvalidQueryException.class,
+ "SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE v IN (200, 250, 300)");
Review comment:
We can use a `%s` placeholder for the query
```suggestion
StatementRestrictions.REQUIRES_ALLOW_FILTERING_MESSAGE,
InvalidQueryException.class,
"SELECT * FROM %s WHERE v IN (200, 250,
300)");
```
##########
File path:
test/unit/org/apache/cassandra/cql3/validation/operations/SelectMultiColumnRelationTest.java
##########
@@ -1930,4 +1930,38 @@ public void testInvalidColumnNames() throws Throwable
assertInvalidMessage("Undefined column name e", "SELECT c AS e FROM %s
WHERE (b, e) IN ((0, 1), (2, 4))");
assertInvalidMessage("Undefined column name e", "SELECT c AS e FROM %s
WHERE (b, e) > (0, 1) and b <= 2");
}
+
+ @Test
+ public void testInRestrictionsWithAllowFiltering() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, c1 text, c2 int, v int, primary
key(pk, c1, c2))");
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "0",
0, 3);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "1",
0, 4);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "1",
1, 5);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "2",
1, 6);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "2",
2, 7);
+
+ assertRows(execute("SELECT * FROM %s WHERE (c2) IN ((?), (?)) ALLOW
FILTERING", 1, 3),
+ row(1, "1", 1, 5),
+ row(1, "2", 1, 6));
+
+ assertRows(execute("SELECT * FROM %s WHERE c2 IN (?, ?) ALLOW
FILTERING", 1, 3),
+ row(1, "1", 1, 5),
+ row(1, "2", 1, 6));
+ }
+
+ @Test
+ public void testInRestrictionsWithIndex() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, c1 text, c2 int, c3 int, v int,
primary key(pk, c1, c2, c3))");
+ createIndex("CREATE INDEX ON %s (c3)");
+ execute("INSERT INTO %s (pk, c1, c2, c3, v) values (?, ?, ?, ?, ?)",
1, "0", 0, 0, 3);
+ execute("INSERT INTO %s (pk, c1, c2, c3, v) values (?, ?, ?, ?, ?)",
1, "1", 0, 0, 4);
+ execute("INSERT INTO %s (pk, c1, c2, c3, v) values (?, ?, ?, ?, ?)",
1, "1", 1, 0, 5);
+ execute("INSERT INTO %s (pk, c1, c2, c3, v) values (?, ?, ?, ?, ?)",
1, "2", 1, 0, 6);
+ execute("INSERT INTO %s (pk, c1, c2, c3, v) values (?, ?, ?, ?, ?)",
1, "2", 2, 0, 7);
Review comment:
Probably we don't need the insertions if we are only going to verify
query validation.
##########
File path: src/java/org/apache/cassandra/index/sasi/conf/ColumnIndex.java
##########
@@ -223,10 +223,10 @@ public boolean supports(Operator op)
Op operator = Op.valueOf(op);
return !(isTokenized && operator == Op.EQ) // EQ is only applicable to
non-tokenized indexes
+ && operator != Op.IN // IN operator are not supported
Review comment:
```suggestion
&& operator != Op.IN // IN operator is not supported
```
##########
File path:
test/unit/org/apache/cassandra/cql3/validation/operations/SelectMultiColumnRelationTest.java
##########
@@ -1930,4 +1930,38 @@ public void testInvalidColumnNames() throws Throwable
assertInvalidMessage("Undefined column name e", "SELECT c AS e FROM %s
WHERE (b, e) IN ((0, 1), (2, 4))");
assertInvalidMessage("Undefined column name e", "SELECT c AS e FROM %s
WHERE (b, e) > (0, 1) and b <= 2");
}
+
+ @Test
+ public void testInRestrictionsWithAllowFiltering() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, c1 text, c2 int, v int, primary
key(pk, c1, c2))");
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "0",
0, 3);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "1",
0, 4);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "1",
1, 5);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "2",
1, 6);
+ execute("INSERT INTO %s (pk, c1, c2, v) values (?, ?, ?, ?)", 1, "2",
2, 7);
+
+ assertRows(execute("SELECT * FROM %s WHERE (c2) IN ((?), (?)) ALLOW
FILTERING", 1, 3),
+ row(1, "1", 1, 5),
+ row(1, "2", 1, 6));
+
+ assertRows(execute("SELECT * FROM %s WHERE c2 IN (?, ?) ALLOW
FILTERING", 1, 3),
+ row(1, "1", 1, 5),
+ row(1, "2", 1, 6));
+ }
+
+ @Test
+ public void testInRestrictionsWithIndex() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, c1 text, c2 int, c3 int, v int,
primary key(pk, c1, c2, c3))");
+ createIndex("CREATE INDEX ON %s (c3)");
Review comment:
The test produces the same `"IN restrictions are not supported on
indexed columns"` error if we don't create any index. Indeed, the index is
never selected because of the rejection of the `IN` operator by
`Index#supportsExpression`, and it's
`ClusteringColumnRestrictions#handleInFilter` what leads us to the code raising
the exception.
If we can't process this type of multicolumn restrictions in the row filter
maybe the error message should be something like `"Multicolumn IN filters are
not supported"`, or something like that. We should also add an equivalent test
without the index.
I guess that at some point CEP-7 will add support for OR conditions in the
row filter, I think that then we'll be able to revisit this. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [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]
