You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/08/17 17:38:13 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #1141: CASSANDRA-15135 - validate sasi analyzer
adelapena commented on a change in pull request #1141:
URL: https://github.com/apache/cassandra/pull/1141#discussion_r690552925
##########
File path: src/java/org/apache/cassandra/index/sasi/analyzer/AbstractAnalyzer.java
##########
@@ -38,6 +39,10 @@ public void remove()
throw new UnsupportedOperationException();
}
+ public void validate(Map<String, String> options, AbstractType validator) throws ConfigurationException
Review comment:
Nit: can we use `AbstractType<?>`?
##########
File path: src/java/org/apache/cassandra/index/sasi/analyzer/DelimiterAnalyzer.java
##########
@@ -56,14 +57,16 @@ public ByteBuffer next()
return iter.next();
}
+ public void validate(Map<String, String> options, AbstractType validator) throws ConfigurationException
Review comment:
Nit: can we mark the method with `@Override`?
##########
File path: src/java/org/apache/cassandra/index/sasi/analyzer/NonTokenizingAnalyzer.java
##########
@@ -56,6 +57,14 @@
private ByteBuffer input;
private boolean hasNext = false;
+ public void validate(Map<String, String> options, AbstractType validator) throws ConfigurationException
+ {
+ if (options.containsKey(NonTokenizingOptions.CASE_SENSITIVE) && (options.containsKey(NonTokenizingOptions.NORMALIZE_LOWERCASE)
+ || options.containsKey(NonTokenizingOptions.NORMALIZE_UPPERCASE)))
+ throw new ConfigurationException("case_sensitive option cannot be specified together " +
+ "with either normalize_lowercase or normalize_uppercase");
Review comment:
Nit: maybe we can try to change the original formatting to fit into the line length:
```suggestion
if (options.containsKey(NonTokenizingOptions.CASE_SENSITIVE) &&
(options.containsKey(NonTokenizingOptions.NORMALIZE_LOWERCASE) ||
options.containsKey(NonTokenizingOptions.NORMALIZE_UPPERCASE)))
throw new ConfigurationException("case_sensitive option cannot be specified together " +
"with either normalize_lowercase or normalize_uppercase");
```
##########
File path: src/java/org/apache/cassandra/index/sasi/analyzer/NonTokenizingAnalyzer.java
##########
@@ -56,6 +57,14 @@
private ByteBuffer input;
private boolean hasNext = false;
+ public void validate(Map<String, String> options, AbstractType validator) throws ConfigurationException
Review comment:
Nit: can we mark the method with `@Override`?
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java
##########
@@ -2484,6 +2484,40 @@ public void testAnalyzerValidation()
});
}
+ @Test
+ public void testIllegalArgumentsForAnalyzerShouldFail()
+ {
+ String baseTable = "illegal_argument_test";
+ String indexName = "illegal_index";
+ QueryProcessor.executeOnceInternal(String.format("CREATE KEYSPACE IF NOT EXISTS %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", KS_NAME));
+ QueryProcessor.executeOnceInternal(String.format("CREATE TABLE IF NOT EXISTS %s.%s (k int primary key, v text);", KS_NAME, baseTable));
+
+ try
+ {
+ QueryProcessor.executeOnceInternal(String.format("CREATE CUSTOM INDEX IF NOT EXISTS %s ON %s.%s(v) " +
+ "USING 'org.apache.cassandra.index.sasi.SASIIndex' WITH OPTIONS = { 'mode' : 'CONTAINS', " +
+ "'analyzer_class': 'org.apache.cassandra.index.sasi.analyzer.NonTokenizingAnalyzer', " +
+ "'case_sensitive': 'false'," +
+ "'normalize_uppercase': 'true'};",
+ indexName, KS_NAME, baseTable));
+
+ Assert.fail("creation of index analyzer with illegal options should fail");
+ }
+ catch (ConfigurationException e)
+ {
+ //correct behaviour
+ //confirm that it wasn't written to the schema
+ Assert.assertTrue(QueryProcessor.executeOnceInternal(String.format("SELECT * FROM system_schema.indexes WHERE keyspace_name = '%s' " +
+ "and table_name = '%s' and index_name = '%s';", KS_NAME, baseTable, indexName)).isEmpty());
Review comment:
Nit: not a big deal, but we could prevent the NPE warning for example using `assertThat`:
```suggestion
String query = String.format("SELECT * FROM system_schema.indexes WHERE keyspace_name = '%s' " +
"AND table_name = '%s' AND index_name = '%s'",
KS_NAME, baseTable, indexName);
Assertions.assertThat(QueryProcessor.executeOnceInternal(query)).isEmpty();
```
##########
File path: src/java/org/apache/cassandra/index/sasi/analyzer/DelimiterAnalyzer.java
##########
@@ -56,14 +57,16 @@ public ByteBuffer next()
return iter.next();
}
+ public void validate(Map<String, String> options, AbstractType validator) throws ConfigurationException
+ {
+ if (!VALID_ANALYZABLE_TYPES.containsKey(validator))
+ throw new ConfigurationException(String.format("Only text types supported, got %s", validator));
Review comment:
I think this error is unreachable. The new `AbstractAnalyzer#validate` method is called exclusively from `IndexMode#validateAnalyzer`, which already validates the type with a call to `AbstractAnalyzer#isCompatibleWith`.
This makes me thing that probably we don't need the cell type argument on `AbstractAnalyzer#validate`, or alternatively we could consider consider making `AbstractAnalyzer#validate` consistently responsible for checking the cell type so `IndexMode#validateAnalyzer` doesn't have to call `AbstractAnalyzer#isCompatibleWith`. 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: 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