Skip to content

Commit

Permalink
validateOptions treats analyzed and un-analyzed indexes as distinct, …
Browse files Browse the repository at this point in the history
…testTwoIndexes passes
  • Loading branch information
jbellis committed Dec 12, 2024
1 parent 907a2ee commit 3315a12
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 13 deletions.
30 changes: 18 additions & 12 deletions src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java
Original file line number Diff line number Diff line change
Expand Up @@ -297,20 +297,26 @@ public static Map<String, String> validateOptions(Map<String, String> options, T
throw new InvalidRequestException("Failed to retrieve target column for: " + targetColumn);
}

// In order to support different index target on non-frozen map, ie. KEYS, VALUE, ENTRIES, we need to put index
// name as part of index file name instead of column name. We only need to check that the target is different
// between indexes. This will only allow indexes in the same column with a different IndexTarget.Type.
//
// Note that: "metadata.indexes" already includes current index
if (metadata.indexes.stream().filter(index -> index.getIndexClassName().equals(StorageAttachedIndex.class.getName()))
.map(index -> TargetParser.parse(metadata, index.options.get(IndexTarget.TARGET_OPTION_NAME)))
.filter(Objects::nonNull).filter(t -> t.equals(target)).count() > 1)
{
throw new InvalidRequestException("Cannot create more than one storage-attached index on the same column: " + target.left);
}
// Check for duplicate indexes considering both target and analyzer configuration
boolean isAnalyzed = AbstractAnalyzer.isAnalyzed(options);
long duplicateCount = metadata.indexes.stream()
.filter(index -> index.getIndexClassName().equals(StorageAttachedIndex.class.getName()))
.filter(index -> {
// Indexes on the same column with different target (KEYS, VALUES, ENTRIES)
// are allowed on non-frozen Maps
var existingTarget = TargetParser.parse(metadata, index.options.get(IndexTarget.TARGET_OPTION_NAME));
if (existingTarget == null || !existingTarget.equals(target))
return false;
// Also allow different indexes if one is analyzed and the other isn't
return isAnalyzed == AbstractAnalyzer.isAnalyzed(index.options);
})
.count();
// >1 because "metadata.indexes" already includes current index
if (duplicateCount > 1)
throw new InvalidRequestException(String.format("Cannot create duplicate storage-attached index on column: %s", target.left));

// Analyzer is not supported against PK columns
if (AbstractAnalyzer.isAnalyzed(options))
if (isAnalyzed)
{
for (ColumnMetadata column : metadata.primaryKeyColumns())
{
Expand Down
9 changes: 8 additions & 1 deletion test/unit/org/apache/cassandra/index/sai/cql/BM25Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,18 @@ public void testTwoIndexes() throws Throwable
// create un-analyzed index
createTable("CREATE TABLE %s (k int PRIMARY KEY, v text)");
createIndex("CREATE CUSTOM INDEX ON %s(v) USING 'org.apache.cassandra.index.sai.StorageAttachedIndex'");

execute("INSERT INTO %s (k, v) VALUES (1, 'apple')");

// BM25 should fail with only an equality index
assertThatThrownBy(() -> execute("SELECT k FROM %s WHERE v : 'apple' ORDER BY v BM25 OF 'apple' LIMIT 3"))
.isInstanceOf(InvalidRequestException.class)
.hasMessage("BM25 ordering on column v requires an analyzed index");

// create analyzed index
analyzeIndex();
// BM25 query should work now
var result = execute("SELECT k FROM %s WHERE v : 'apple' ORDER BY v BM25 OF 'apple' LIMIT 3");
assertRows(result, row(1));
}

@Test
Expand Down

0 comments on commit 3315a12

Please sign in to comment.