From b265b4658e007b6943d543a11c609b7ba5fd979f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20de=20la=20Pe=C3=B1a?= Date: Wed, 19 Jul 2023 17:40:50 +0100 Subject: [PATCH] Forbid SAI indexes with analysis options on primary key columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit patch by Andrés de la Peña; reviewed by Caleb Rackliffe for CASSANDRA-18782 --- CHANGES.txt | 1 + .../index/sai/StorageAttachedIndex.java | 11 ++++++- .../index/sai/analyzer/AbstractAnalyzer.java | 12 +++++-- .../sai/analyzer/NonTokenizingOptions.java | 5 +++ .../sai/cql/StorageAttachedIndexDDLTest.java | 31 +++++++++++++++++++ 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 262a2edcb6..68515312fe 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0-alpha2 + * Forbid SAI indexes with analysis options on primary key columns (CASSANDRA-18782) Merged from 4.1: Merged from 4.0: Merged from 3.11: diff --git a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java index e8c4b53f4e..805a6b65e1 100644 --- a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java +++ b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java @@ -48,6 +48,7 @@ import org.slf4j.LoggerFactory; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.CQL3Type; +import org.apache.cassandra.cql3.CqlBuilder; import org.apache.cassandra.cql3.Operator; import org.apache.cassandra.cql3.statements.schema.IndexTarget; import org.apache.cassandra.db.CassandraWriteContext; @@ -96,6 +97,9 @@ import org.apache.cassandra.utils.concurrent.OpOrder; public class StorageAttachedIndex implements Index { public static final String NAME = "sai"; + + @VisibleForTesting + public static final String ANALYSIS_ON_KEY_COLUMNS_MESSAGE = "Analysis options are not supported on primary key columns, but found "; private static final Logger logger = LoggerFactory.getLogger(StorageAttachedIndex.class); @@ -253,7 +257,12 @@ public class StorageAttachedIndex implements Index throw new InvalidRequestException("Unsupported type: " + type.asCQL3Type()); } - AbstractAnalyzer.fromOptions(type, options); + Map analysisOptions = AbstractAnalyzer.getAnalyzerOptions(options); + if (target.left.isPrimaryKeyColumn() && !analysisOptions.isEmpty()) + { + throw new InvalidRequestException(ANALYSIS_ON_KEY_COLUMNS_MESSAGE + new CqlBuilder().append(analysisOptions)); + } + AbstractAnalyzer.fromOptions(type, analysisOptions); return Collections.emptyMap(); } diff --git a/src/java/org/apache/cassandra/index/sai/analyzer/AbstractAnalyzer.java b/src/java/org/apache/cassandra/index/sai/analyzer/AbstractAnalyzer.java index 1f5e339657..da0b2a4d19 100644 --- a/src/java/org/apache/cassandra/index/sai/analyzer/AbstractAnalyzer.java +++ b/src/java/org/apache/cassandra/index/sai/analyzer/AbstractAnalyzer.java @@ -22,6 +22,7 @@ import java.nio.ByteBuffer; import java.util.Iterator; import java.util.Map; import java.util.NoSuchElementException; +import java.util.stream.Collectors; import org.apache.cassandra.db.marshal.AbstractType; import org.apache.cassandra.db.marshal.StringType; @@ -103,9 +104,14 @@ public abstract class AbstractAnalyzer implements Iterator private static boolean hasNonTokenizingOptions(Map options) { - return options.get(NonTokenizingOptions.ASCII) != null || - options.containsKey(NonTokenizingOptions.CASE_SENSITIVE) || - options.containsKey(NonTokenizingOptions.NORMALIZE); + return options.keySet().stream().anyMatch(NonTokenizingOptions::hasOption); + } + + public static Map getAnalyzerOptions(Map options) + { + return options.entrySet().stream() + .filter(e -> NonTokenizingOptions.hasOption(e.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); } } diff --git a/src/java/org/apache/cassandra/index/sai/analyzer/NonTokenizingOptions.java b/src/java/org/apache/cassandra/index/sai/analyzer/NonTokenizingOptions.java index ab6485acf5..66c7740a53 100644 --- a/src/java/org/apache/cassandra/index/sai/analyzer/NonTokenizingOptions.java +++ b/src/java/org/apache/cassandra/index/sai/analyzer/NonTokenizingOptions.java @@ -65,6 +65,11 @@ public class NonTokenizingOptions this.normalized = normalized; } + static boolean hasOption(String option) + { + return option.equals(NORMALIZE) || option.equals(CASE_SENSITIVE) || option.equals(ASCII); + } + public static class OptionsBuilder { private boolean caseSensitive = true; diff --git a/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java b/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java index b3d57d95a0..6dedd5ed2d 100644 --- a/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java +++ b/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java @@ -24,11 +24,13 @@ import java.io.IOException; import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.LongStream; +import com.google.common.collect.ImmutableMap; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -40,6 +42,7 @@ import com.datastax.driver.core.exceptions.ReadFailureException; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.CQL3Type; import org.apache.cassandra.cql3.ColumnIdentifier; +import org.apache.cassandra.cql3.CqlBuilder; import org.apache.cassandra.cql3.restrictions.IndexRestrictions; import org.apache.cassandra.cql3.statements.schema.CreateIndexStatement; import org.apache.cassandra.db.ColumnFamilyStore; @@ -58,6 +61,7 @@ import org.apache.cassandra.index.sai.IndexContext; import org.apache.cassandra.index.sai.SAITester; import org.apache.cassandra.index.sai.StorageAttachedIndex; import org.apache.cassandra.index.sai.StorageAttachedIndexBuilder; +import org.apache.cassandra.index.sai.analyzer.NonTokenizingOptions; import org.apache.cassandra.index.sai.disk.format.IndexComponent; import org.apache.cassandra.index.sai.disk.format.Version; import org.apache.cassandra.index.sai.disk.v1.segment.SegmentBuilder; @@ -417,6 +421,33 @@ public class StorageAttachedIndexDDLTest extends SAITester assertEquals(1, execute("SELECT id FROM %s WHERE val = 'eppinger'").size()); } + @Test + public void shouldRejectAnalysisOnPrimaryKeyColumns() + { + createTable("CREATE TABLE %s (k1 text, k2 text, c1 text, c2 text, PRIMARY KEY((k1, k2), c1, c2))"); + + for (String column : Arrays.asList("k1", "k2", "c1", "c2")) + { + for (String enabled : Arrays.asList("true", "false")) + { + assertRejectsAnalysisOnPrimaryKeyColumns(column, ImmutableMap.of(NonTokenizingOptions.NORMALIZE, enabled)); + assertRejectsAnalysisOnPrimaryKeyColumns(column, ImmutableMap.of(NonTokenizingOptions.CASE_SENSITIVE, enabled)); + assertRejectsAnalysisOnPrimaryKeyColumns(column, ImmutableMap.of(NonTokenizingOptions.ASCII, enabled)); + assertRejectsAnalysisOnPrimaryKeyColumns(column, ImmutableMap.of(NonTokenizingOptions.NORMALIZE, enabled, + NonTokenizingOptions.CASE_SENSITIVE, enabled, + NonTokenizingOptions.ASCII, enabled)); + } + } + } + + private void assertRejectsAnalysisOnPrimaryKeyColumns(String column, Map optionsMap) + { + String options = new CqlBuilder().append(optionsMap).toString(); + Assertions.assertThatThrownBy(() -> createIndex("CREATE INDEX ON %s(" + column + ") USING 'sai' WITH OPTIONS = " + options)) + .hasRootCauseInstanceOf(InvalidRequestException.class) + .hasRootCauseMessage(StorageAttachedIndex.ANALYSIS_ON_KEY_COLUMNS_MESSAGE + options); + } + @Test public void shouldCreateIndexOnReversedType() {