diff --git a/CHANGES.txt b/CHANGES.txt index dc0bfeb9d5..d1a5abe729 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0-beta1 + * SAI fixes for composite partitions, and static and non-static rows intersections (CASSANDRA-19034) * Improve SAI IndexContext handling of indexed and non-indexed columns in queries (CASSANDRA-18166) * Fixed bug where UnifiedCompactionTask constructor was calling the wrong base constructor of CompactionTask (CASSANDRA-18757) * Fix SAI unindexed contexts not considering CONTAINS KEY (CASSANDRA-19040) diff --git a/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java b/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java index d94ac9118e..abfd6c9e3f 100644 --- a/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java +++ b/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java @@ -388,14 +388,11 @@ public class StorageAttachedIndexSearcher implements Index.Searcher { Row staticRow = partition.staticRow(); - // We want to short-circuit the filtering of the whole partition if the static row - // satisfies the filter. If that is the case we just need to return the whole partition. - queryContext.rowsFiltered++; - if (tree.isSatisfiedBy(partition.partitionKey(), staticRow, staticRow)) - return partition; - List clusters = new ArrayList<>(); + // We need to filter the partition rows before filtering on the static row. If this is done in the other + // order then we get incorrect results if we are filtering on a partition key index on a table with a + // composite partition key. while (partition.hasNext()) { Unfiltered row = partition.next(); @@ -407,6 +404,15 @@ public class StorageAttachedIndexSearcher implements Index.Searcher } } + if (clusters.isEmpty()) + { + queryContext.rowsFiltered++; + if (tree.isSatisfiedBy(key.partitionKey(), staticRow, staticRow)) + { + clusters.add(staticRow); + } + } + /* * If {@code clusters} is empty, which means either all clustering row and static row pairs failed, * or static row and static row pair failed. In both cases, we should not return any partition. diff --git a/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java b/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java index 7918cbad0c..f3492927eb 100644 --- a/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java +++ b/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java @@ -309,9 +309,10 @@ public interface PrimaryKey extends Comparable, ByteComparable int cmp = super.compareTo(o); if (cmp != 0 || o.kind() == Kind.TOKEN || o.kind() == Kind.SKINNY) return cmp; - // The static clustering comes first in the sort order of if the other key has static clustering we - // are equals otherwise we are less than the other - return o.kind() == Kind.STATIC ? 0 : -1; + // At this point the other key is in the same partition as this static key so is equal to it. This + // has to be the case because otherwise, intersections between static column indexes and ordinary + // indexes will fail. + return 0; } @Override @@ -368,9 +369,10 @@ public interface PrimaryKey extends Comparable, ByteComparable int cmp = super.compareTo(o); if (cmp != 0 || o.kind() == Kind.TOKEN || o.kind() == Kind.SKINNY) return cmp; - // At this point we will be greater than other if it is static + // At this point this key is in the same partition as the other key so if the other key is a static + // key then it must be equal to it. See comment in the compareTo for static keys above. if (o.kind() == Kind.STATIC) - return 1; + return 0; return clusteringComparator.compare(clustering(), o.clustering()); } diff --git a/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java b/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java index 0e488b2507..99fcdf8319 100644 --- a/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java +++ b/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java @@ -17,88 +17,92 @@ */ package org.apache.cassandra.index.sai.cql; -import org.junit.Before; import org.junit.Test; +import org.apache.cassandra.cql3.restrictions.StatementRestrictions; import org.apache.cassandra.index.sai.SAITester; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - public class CompositePartitionKeyIndexTest extends SAITester { - @Before - public void createTableAndIndex() + @Test + public void testCompositePartitionIndex() throws Throwable { createTable("CREATE TABLE %s (pk1 int, pk2 text, val int, PRIMARY KEY((pk1, pk2)))"); createIndex("CREATE INDEX ON %s(pk1) USING 'sai'"); createIndex("CREATE INDEX ON %s(pk2) USING 'sai'"); - disableCompaction(); - } - - private void insertData1() - { execute("INSERT INTO %s (pk1, pk2, val) VALUES (1, '1', 1)"); execute("INSERT INTO %s (pk1, pk2, val) VALUES (2, '2', 2)"); execute("INSERT INTO %s (pk1, pk2, val) VALUES (3, '3', 3)"); - } - - private void insertData2() - { execute("INSERT INTO %s (pk1, pk2, val) VALUES (4, '4', 4)"); execute("INSERT INTO %s (pk1, pk2, val) VALUES (5, '5', 5)"); execute("INSERT INTO %s (pk1, pk2, val) VALUES (6, '6', 6)"); + + beforeAndAfterFlush(() -> { + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 = 2"), + expectedRow(2)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 > 1"), + expectedRow(2), + expectedRow(3), + expectedRow(4), + expectedRow(5), + expectedRow(6)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 >= 3"), + expectedRow(3), + expectedRow(4), + expectedRow(5), + expectedRow(6)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 < 3"), + expectedRow(1), + expectedRow(2)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 <= 3"), + expectedRow(1), + expectedRow(2), + expectedRow(3)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk2 = '2'"), + expectedRow(2)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 > 1 AND pk2 = '2'"), + expectedRow(2)); + + assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 = -1 AND pk2 = '2'")); + + assertInvalidMessage(StatementRestrictions.REQUIRES_ALLOW_FILTERING_MESSAGE, "SELECT * FROM %s WHERE pk1 = -1 AND val = 2"); + }); } @Test - public void queryFromMemtable() + public void testFilterWithIndexForContains() throws Throwable { - insertData1(); - insertData2(); - runQueries(); + createTable("CREATE TABLE %s (k1 int, k2 int, v set, PRIMARY KEY ((k1, k2)))"); + createIndex("CREATE INDEX ON %s(k2) USING 'sai'"); + + execute("INSERT INTO %s (k1, k2, v) VALUES (?, ?, ?)", 0, 0, set(1, 2, 3)); + execute("INSERT INTO %s (k1, k2, v) VALUES (?, ?, ?)", 0, 1, set(2, 3, 4)); + execute("INSERT INTO %s (k1, k2, v) VALUES (?, ?, ?)", 1, 0, set(3, 4, 5)); + execute("INSERT INTO %s (k1, k2, v) VALUES (?, ?, ?)", 1, 1, set(4, 5, 6)); + + beforeAndAfterFlush(() -> { + assertRows(execute("SELECT * FROM %s WHERE k2 = ?", 1), + row(0, 1, set(2, 3, 4)), + row(1, 1, set(4, 5, 6)) + ); + + assertRows(execute("SELECT * FROM %s WHERE k2 = ? AND v CONTAINS ? ALLOW FILTERING", 1, 6), + row(1, 1, set(4, 5, 6)) + ); + + assertEmpty(execute("SELECT * FROM %s WHERE k2 = ? AND v CONTAINS ? ALLOW FILTERING", 1, 7)); + }); } - private Object[] expectedRow(int index) { + private Object[] expectedRow(int index) + { return row(index, Integer.toString(index), index); } - - private void runQueries() - { - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 = 2"), - expectedRow(2)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 > 1"), - expectedRow(2), - expectedRow(3), - expectedRow(4), - expectedRow(5), - expectedRow(6)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 >= 3"), - expectedRow(3), - expectedRow(4), - expectedRow(5), - expectedRow(6)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 < 3"), - expectedRow(1), - expectedRow(2)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 <= 3"), - expectedRow(1), - expectedRow(2), - expectedRow(3)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk2 = '2'"), - expectedRow(2)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 > 1 AND pk2 = '2'"), - expectedRow(2)); - - assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE pk1 = -1 AND pk2 = '2'")); - - assertThatThrownBy(()->execute("SELECT * FROM %s WHERE pk1 = -1 AND val = 2")) - .hasMessageContaining("use ALLOW FILTERING"); - - } } diff --git a/test/unit/org/apache/cassandra/index/sai/cql/StaticColumnIndexTest.java b/test/unit/org/apache/cassandra/index/sai/cql/StaticColumnIndexTest.java new file mode 100644 index 0000000000..0bdee106fd --- /dev/null +++ b/test/unit/org/apache/cassandra/index/sai/cql/StaticColumnIndexTest.java @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.index.sai.cql; + +import org.junit.Test; + +import org.apache.cassandra.index.sai.SAITester; + +public class StaticColumnIndexTest extends SAITester +{ + @Test + public void staticIndexReturnsAllRowsInPartition() throws Throwable + { + createTable("CREATE TABLE %s (pk int, ck int, val1 int static, val2 int, PRIMARY KEY(pk, ck))"); + createIndex("CREATE INDEX ON %s(val1) USING 'sai'"); + + execute("INSERT INTO %s(pk, ck, val1, val2) VALUES(?, ?, ?, ?)", 1, 1, 2, 1); + execute("INSERT INTO %s(pk, ck, val2) VALUES(?, ?, ?)", 1, 2, 2); + execute("INSERT INTO %s(pk, ck, val2) VALUES(?, ?, ?)", 1, 3, 3); + + beforeAndAfterFlush(() -> assertRows(execute("SELECT pk, ck, val1, val2 FROM %s WHERE val1 = 2"), + row(1, 1, 2, 1), row(1, 2, 2, 2), row(1, 3, 2, 3))); + } + + @Test + public void staticIndexAndNonStaticIndex() throws Throwable + { + createTable("CREATE TABLE %s (pk int, ck int, val1 int static, val2 int, PRIMARY KEY(pk, ck))"); + createIndex("CREATE INDEX ON %s(val1) USING 'sai'"); + createIndex("CREATE INDEX ON %s(val2) USING 'sai'"); + + execute("INSERT INTO %s(pk, ck, val1, val2) VALUES(?, ?, ?, ?)", 1, 1, 20, 1000); + execute("INSERT INTO %s(pk, ck, val2) VALUES(?, ?, ?)", 1, 2, 2000); + execute("INSERT INTO %s(pk, ck, val1, val2) VALUES(?, ?, ?, ?)", 2, 1, 40, 2000); + + beforeAndAfterFlush(() -> assertRows(execute("SELECT pk, ck, val1, val2 FROM %s WHERE val1 = 20 AND val2 = 2000 ALLOW FILTERING"), + row(1, 2, 20, 2000))); + } +} diff --git a/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java b/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java index c64372feab..1c4b7d40ba 100644 --- a/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java +++ b/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java @@ -83,7 +83,8 @@ public class PrimaryKeyTest extends AbstractPrimaryKeyTester public void simplePartitonStaticAndSingleClusteringAscTest() { PrimaryKey.Factory factory = new PrimaryKey.Factory(Murmur3Partitioner.instance, simplePartitionStaticAndSingleClusteringAsc.comparator); - int rows = nextInt(10, 100); +// int rows = nextInt(10, 100); + int rows = 10; PrimaryKey[] keys = new PrimaryKey[rows]; int partition = 0; int clustering = 0; @@ -372,9 +373,14 @@ public class PrimaryKeyTest extends AbstractPrimaryKeyTester assertCompareToAndEquals(key, key, 0); assertCompareToAndEquals(tokenOnlyKey, tokenOnlyKey, 0); + // StaticPrimaryKey is a special case. All other keys in the partition are equal to it + boolean staticComparison = key.kind() == PrimaryKey.Kind.STATIC; + boolean inPartition = staticComparison; for (int comparisonIndex = index + 1; comparisonIndex < keys.length; comparisonIndex++) { - assertCompareToAndEquals(key, keys[comparisonIndex], -1); + if (staticComparison && keys[comparisonIndex].kind() == PrimaryKey.Kind.STATIC) + inPartition = false; + assertCompareToAndEquals(key, keys[comparisonIndex], inPartition ? 0 : -1); assertCompareToAndEquals(tokenOnlyKey, keys[comparisonIndex], tokenOnlyKey.token().equals(keys[comparisonIndex].token()) ? 0 : -1); } }