From 225eb2feeb6ba856c5bf3f46fd0534d074539433 Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Thu, 23 Jan 2025 11:52:38 -0600 Subject: [PATCH] Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-20238 --- CHANGES.txt | 1 + .../v1/postings/PostingListRangeIterator.java | 4 +- .../sai/iterators/KeyRangeConcatIterator.java | 6 +-- .../KeyRangeIntersectionIterator.java | 13 +++--- .../index/sai/iterators/KeyRangeIterator.java | 15 ++++-- .../sai/iterators/KeyRangeListIterator.java | 2 +- .../sai/iterators/KeyRangeUnionIterator.java | 4 +- .../sai/memory/InMemoryKeyRangeIterator.java | 4 +- .../plan/StorageAttachedIndexSearcher.java | 2 +- .../cassandra/index/sai/utils/PrimaryKey.java | 44 +++++++++--------- .../cql/CompositePartitionKeyIndexTest.java | 46 +++++++++++++++++++ .../index/sai/iterators/LongIterator.java | 4 +- .../index/sai/utils/PrimaryKeyTest.java | 7 +-- 13 files changed, 101 insertions(+), 51 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 1019b4d5c3..84a20e6c71 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0.3 + * Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys (CASSANDRA-20238) * Make sure we can set parameters when configuring CassandraCIDRAuthorizer (CASSANDRA-20220) * Add selected SAI index state and query performance metrics to nodetool tablestats (CASSANDRA-20026) * Remove v30 and v3X from 5.x in-JVM upgrade tests (CASSANDRA-20103) diff --git a/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java b/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java index 4f05e2a171..813017db8d 100644 --- a/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java +++ b/src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java @@ -89,7 +89,7 @@ public class PostingListRangeIterator extends KeyRangeIterator @Override protected void performSkipTo(PrimaryKey nextKey) { - if (skipToKey != null && skipToKey.compareTo(nextKey) > 0) + if (skipToKey != null && skipToKey.compareTo(nextKey, false) > 0) return; skipToKey = nextKey; @@ -137,7 +137,7 @@ public class PostingListRangeIterator extends KeyRangeIterator private boolean exhausted() { - return needsSkipping && skipToKey.compareTo(getMaximum()) > 0; + return needsSkipping && skipToKey.compareTo(getMaximum(), false) > 0; } /** diff --git a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java index bb83f1ea81..cd47ff219b 100644 --- a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java +++ b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java @@ -59,10 +59,10 @@ public class KeyRangeConcatIterator extends KeyRangeIterator { KeyRangeIterator currentIterator = ranges.get(current); - if (currentIterator.hasNext() && currentIterator.peek().compareTo(nextKey) >= 0) + if (currentIterator.hasNext() && currentIterator.peek().compareTo(nextKey, false) >= 0) break; - if (currentIterator.getMaximum().compareTo(nextKey) >= 0) + if (currentIterator.getMaximum().compareTo(nextKey, false) >= 0) { currentIterator.skipTo(nextKey); break; @@ -178,7 +178,7 @@ public class KeyRangeConcatIterator extends KeyRangeIterator { min = range.getMinimum(); } - else if (count > 0 && max.compareTo(range.getMinimum()) > 0) + else if (count > 0 && max.compareTo(range.getMinimum(), false) > 0) { throw new IllegalArgumentException(String.format(MUST_BE_SORTED_ERROR, max, range.getMinimum())); } diff --git a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java index 5391dd75ed..e820b2fc4b 100644 --- a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java +++ b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java @@ -74,7 +74,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator // compare to other keys only by partition.) This loop continues until all iterators point to the same key, // or if we run out of keys on any of them, or if we exceed the maximum key. // There is no point in iterating after maximum, because no keys will match beyond that point. - while (highestKey != null && highestKey.compareTo(getMaximum()) <= 0) + while (highestKey != null && highestKey.compareTo(getMaximum(), false) <= 0) { // Try to advance all iterators to the highest key seen so far. // Once this inner loop finishes normally, all iterators are guaranteed to be at the same value. @@ -83,7 +83,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator if (!range.hasNext()) return endOfData(); - if (range.peek().compareTo(highestKey) < 0) + if (range.peek().compareTo(highestKey, false) < 0) { // If we advance a STATIC key, then we must advance it to the same partition as the highestKey. // Advancing a STATIC key to a WIDE key directly (without throwing away the clustering) would @@ -95,7 +95,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator // We use strict comparison here, since it orders WIDE primary keys after STATIC primary keys // in the same partition. When WIDE keys are present, we want to return them rather than STATIC // keys to avoid retrieving and post-filtering entire partitions. - if (nextKey == null || nextKey.compareToStrict(highestKey) > 0) + if (nextKey == null || nextKey.compareTo(highestKey, true) > 0) { // We jumped over the highest key seen so far, so make it the new highest key. highestKey = nextKey; @@ -105,7 +105,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator // Therefore, restart the inner loop in order to advance the lagging iterators. continue outer; } - assert nextKey.compareTo(highestKey) == 0 : + assert nextKey.compareTo(highestKey, false) == 0 : String.format("Skipped to a key smaller than the target! " + "iterator: %s, target key: %s, returned key: %s", range, highestKey, nextKey); } @@ -161,7 +161,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator { if (!range.hasNext()) return null; - if (range.peek().compareToStrict(max) > 0) + if (range.peek().compareTo(max, true) > 0) max = range.peek(); } return max; @@ -394,6 +394,7 @@ public class KeyRangeIntersectionIterator extends KeyRangeIterator */ private static boolean isDisjointInternal(PrimaryKey min, PrimaryKey max, KeyRangeIterator b) { - return min == null || max == null || b.getMaxKeys() == 0 || min.compareTo(b.getMaximum()) > 0 || (b.hasNext() && b.peek().compareTo(max) > 0); + return min == null || max == null || b.getMaxKeys() == 0 + || min.compareTo(b.getMaximum(), false) > 0 || (b.hasNext() && b.peek().compareTo(max, false) > 0); } } diff --git a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java index 52c390770b..a4ada8ec57 100644 --- a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java +++ b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java @@ -108,10 +108,10 @@ public abstract class KeyRangeIterator extends AbstractGuavaIterator if (state == State.DONE) return; - if (state == State.READY && next.compareTo(nextKey) >= 0) + if (state == State.READY && next.compareTo(nextKey, false) >= 0) return; - if (max.compareTo(nextKey) < 0) + if (max.compareTo(nextKey, false) < 0) { endOfData(); return; @@ -229,7 +229,7 @@ public abstract class KeyRangeIterator extends AbstractGuavaIterator if (a == null) return b; if (b == null) return a; - return a.compareToStrict(b) > 0 ? b : a; + return a.compareTo(b) > 0 ? b : a; } protected static PrimaryKey nullSafeMax(PrimaryKey a, PrimaryKey b) @@ -237,6 +237,13 @@ public abstract class KeyRangeIterator extends AbstractGuavaIterator if (a == null) return b; if (b == null) return a; - return a.compareToStrict(b) > 0 ? a : b; + // The STATIC key sorts before WIDE keys in its partition, but to avoid missing rows while + // intersecting, the STATIC key must override any WIDE key. + if (a.kind() == PrimaryKey.Kind.STATIC && b.kind() == PrimaryKey.Kind.WIDE) + return a.compareTo(b, false) >= 0 ? a : b; + else if (b.kind() == PrimaryKey.Kind.STATIC && a.kind() == PrimaryKey.Kind.WIDE) + return b.compareTo(a, false) >= 0 ? b : a; + + return a.compareTo(b) > 0 ? a : b; } } diff --git a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeListIterator.java b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeListIterator.java index 334e740db8..4bf5d19bc3 100644 --- a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeListIterator.java +++ b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeListIterator.java @@ -50,7 +50,7 @@ public class KeyRangeListIterator extends KeyRangeIterator { while (keyQueue.hasNext()) { - if (keyQueue.peek().compareTo(nextKey) >= 0) + if (keyQueue.peek().compareTo(nextKey, false) >= 0) break; keyQueue.next(); } diff --git a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeUnionIterator.java b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeUnionIterator.java index 7bf73ab534..340333f2af 100644 --- a/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeUnionIterator.java +++ b/src/java/org/apache/cassandra/index/sai/iterators/KeyRangeUnionIterator.java @@ -61,7 +61,7 @@ public class KeyRangeUnionIterator extends KeyRangeIterator { PrimaryKey peeked = range.peek(); - int cmp = candidateKey.compareTo(peeked); + int cmp = candidateKey.compareTo(peeked, false); if (cmp == 0) { @@ -91,7 +91,7 @@ public class KeyRangeUnionIterator extends KeyRangeIterator // Consume the remaining values equal to the candidate key: candidate.next(); } - while (candidate.hasNext() && candidate.peek().compareTo(candidateKey) == 0); + while (candidate.hasNext() && candidate.peek().compareTo(candidateKey, false) == 0); } return candidateKey; diff --git a/src/java/org/apache/cassandra/index/sai/memory/InMemoryKeyRangeIterator.java b/src/java/org/apache/cassandra/index/sai/memory/InMemoryKeyRangeIterator.java index d502afebdd..5131965278 100644 --- a/src/java/org/apache/cassandra/index/sai/memory/InMemoryKeyRangeIterator.java +++ b/src/java/org/apache/cassandra/index/sai/memory/InMemoryKeyRangeIterator.java @@ -70,7 +70,7 @@ public class InMemoryKeyRangeIterator extends KeyRangeIterator if (uniqueKeys) return key; - if (lastKey == null || lastKey.compareTo(key) != 0) + if (lastKey == null || lastKey.compareTo(key, false) != 0) { next = key; lastKey = key; @@ -87,7 +87,7 @@ public class InMemoryKeyRangeIterator extends KeyRangeIterator while (!keys.isEmpty()) { PrimaryKey key = keys.peek(); - if (key.compareTo(nextKey) >= 0) + if (key.compareTo(nextKey, false) >= 0) break; // consume smaller key 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 b235c251ef..858242006a 100644 --- a/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java +++ b/src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java @@ -316,7 +316,7 @@ public class StorageAttachedIndexSearcher implements Index.Searcher */ private boolean isWithinUpperBound(PrimaryKey key) { - return lastPrimaryKey.token().isMinimum() || lastPrimaryKey.compareTo(key) >= 0; + return lastPrimaryKey.token().isMinimum() || lastPrimaryKey.compareTo(key, false) >= 0; } /** 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 4e5081ad0c..3cc5033044 100644 --- a/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java +++ b/src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java @@ -269,10 +269,9 @@ public interface PrimaryKey extends Comparable, ByteComparable // Otherwise, if the other key is token only we can only compare tokens // This is used by the ResultRetriever to skip to the current key range start position // during result retrieval. - if ((cmp != 0) || o.kind() == Kind.TOKEN) + if (cmp != 0 || o.kind() == Kind.TOKEN) return cmp; - // Finally compare the partition keys return partitionKey().compareTo(o.partitionKey()); } @@ -320,24 +319,25 @@ public interface PrimaryKey extends Comparable, ByteComparable } @Override - public int compareTo(PrimaryKey o) + public int compareTo(PrimaryKey o, boolean strict) { int cmp = super.compareTo(o); if (cmp != 0 || o.kind() == Kind.TOKEN || o.kind() == Kind.SKINNY) return cmp; - // 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. + + // If we're comparing strictly, order this STATIC key before a WIDE key, as this corresponds to the + // order of the corresponding row IDs in an on-disk postings list. If we're not being strict, treat + // the keys as being equal, given they are in the same partition. + if (strict && o.kind() == Kind.WIDE) + return -1; + return 0; } @Override - public int compareToStrict(PrimaryKey o) + public int compareTo(PrimaryKey o) { - int cmp = compareTo(o); - // Always order this STATIC key before a WIDE key in the same partition, as this corresponds to the - // order of the corresponding row IDs in an on-disk postings list. - return o.kind() == Kind.WIDE && cmp == 0 ? -1 : cmp; + return compareTo(o, true); } @Override @@ -395,25 +395,25 @@ public interface PrimaryKey extends Comparable, ByteComparable } @Override - public int compareTo(PrimaryKey o) + public int compareTo(PrimaryKey o, boolean strict) { int cmp = super.compareTo(o); if (cmp != 0 || o.kind() == Kind.TOKEN || o.kind() == Kind.SKINNY) return cmp; - // 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 0; + // If we're comparing strictly, order this WIDE key after the STATIC key, as this corresponds to the + // order of the corresponding row IDs in an on-disk postings list. If we're not being strict, treat + // the keys as being equal, given they are in the same partition. + return strict ? 1 : 0; + return clusteringComparator.compare(clustering(), o.clustering()); } @Override - public int compareToStrict(PrimaryKey o) + public int compareTo(PrimaryKey o) { - int cmp = compareTo(o); - // Always order this WIDE key before a STATIC key in the same partition, as this corresponds to the - // order of the corresponding row IDs in an on-disk postings list. - return o.kind() == Kind.STATIC && cmp == 0 ? 1 : cmp; + return compareTo(o, true); } @Override @@ -490,8 +490,8 @@ public interface PrimaryKey extends Comparable, ByteComparable throw new UnsupportedOperationException("Only STATIC and WIDE keys can be converted to STATIC"); } - default int compareToStrict(PrimaryKey o) + default int compareTo(PrimaryKey key, boolean strict) { - return compareTo(o); + return compareTo(key); } } 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 99fcdf8319..f384ca74e4 100644 --- a/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java +++ b/test/unit/org/apache/cassandra/index/sai/cql/CompositePartitionKeyIndexTest.java @@ -17,13 +17,59 @@ */ package org.apache.cassandra.index.sai.cql; +import java.math.BigInteger; + import org.junit.Test; import org.apache.cassandra.cql3.restrictions.StatementRestrictions; +import org.apache.cassandra.db.marshal.SimpleDateType; +import org.apache.cassandra.db.marshal.TimeType; import org.apache.cassandra.index.sai.SAITester; public class CompositePartitionKeyIndexTest extends SAITester { + @Test + public void testIntersectionWithStaticOverlap() throws Throwable + { + createTable("CREATE TABLE %s (pk0 int, pk1 int, ck0 int, s1 int static, v0 int, PRIMARY KEY((pk0, pk1), ck0))"); + createIndex("CREATE INDEX ON %s(pk0) USING 'sai'"); + + execute("UPDATE %s USING TIMESTAMP 1 SET s1 = 0, v0 = 0 WHERE pk0 = 0 AND pk1 = 1 AND ck0 = 0"); + execute("DELETE FROM %s USING TIMESTAMP 2 WHERE pk0 = 0 AND pk1 = 1"); + + // If the STATIC and WIDE PrimaryKey objects in this partition are not compared strictly, the new WIDE key + // will be interpreted as a duplicate and not added to the Memtable-adjacent index. Then, on flush, the row + // corresponding to that WIDE key will be missing from the index. + execute("UPDATE %s USING TIMESTAMP 3 SET v0 = 1 WHERE pk0 = 0 AND pk1 = 1 AND ck0 = 1"); + + beforeAndAfterFlush(() -> assertRows(execute("SELECT * FROM %s WHERE v0 = 1 AND pk0 = 0 ALLOW FILTERING"), row(0, 1, 1, null, 1))); + } + + @Test + public void testIntersectionWithStaticUpdate() throws Throwable + { + createTable("CREATE TABLE %s (pk0 time, pk1 varint, ck0 date, s0 boolean static, s1 text static, v0 boolean, PRIMARY KEY ((pk0, pk1), ck0))"); + createIndex("CREATE INDEX tbl_pk0 ON %s(pk0) USING 'sai'"); + createIndex("CREATE INDEX tbl_s0 ON %s(s0) USING 'sai'"); + + // pk0: 23:15:13.897962392 -> (static clustering, -1296648-01-08) + // s0: false -> (static clustering, -1296648-01-08) + execute("INSERT INTO %s (pk0, pk1, ck0, s0, s1, v0) VALUES ('23:15:13.897962392', -2272, '-1296648-01-08', false, 'ᕊଖꥬ㨢걲映㚃', false)"); + + // pk0: 23:15:13.897962392 -> (static clustering (existing), -1296648-01-08, -1306427-11-21) + // s0: true -> (static clustering, -1306427-11-21) + execute("UPDATE %s SET s0=true, s1='뾕⌒籖' + '鋿紞', v0=true WHERE pk0 = '23:15:13.897962392' AND pk1 = -2272 AND ck0 = '-1306427-11-21'"); + + // Since the value of "true" is never mapped to the clustering -1296648-01-08, the intersection must begin + // at the STATIC key. Otherwise, we will miss the WIDE key for clustering -1296648-01-08. + beforeAndAfterFlush(() -> + assertRows(execute("SELECT * FROM %s WHERE s0 = true AND pk0 = '23:15:13.897962392'"), + row(TimeType.instance.fromString("23:15:13.897962392"), new BigInteger("-2272"), + SimpleDateType.instance.fromString("-1306427-11-21"), true, "뾕⌒籖鋿紞", true), + row(TimeType.instance.fromString("23:15:13.897962392"), new BigInteger("-2272"), + SimpleDateType.instance.fromString("-1296648-01-08"), true, "뾕⌒籖鋿紞", false))); + } + @Test public void testCompositePartitionIndex() throws Throwable { diff --git a/test/unit/org/apache/cassandra/index/sai/iterators/LongIterator.java b/test/unit/org/apache/cassandra/index/sai/iterators/LongIterator.java index 45f655456a..5bbc14cdaa 100644 --- a/test/unit/org/apache/cassandra/index/sai/iterators/LongIterator.java +++ b/test/unit/org/apache/cassandra/index/sai/iterators/LongIterator.java @@ -63,8 +63,8 @@ public class LongIterator extends KeyRangeIterator { for ( ; currentIdx < keys.size(); currentIdx++) { - PrimaryKey token = keys.get(currentIdx); - if (token.compareTo(nextKey) >= 0) + PrimaryKey key = keys.get(currentIdx); + if (key.compareTo(nextKey, false) >= 0) break; } } 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 51edc67389..c64372feab 100644 --- a/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java +++ b/test/unit/org/apache/cassandra/index/sai/utils/PrimaryKeyTest.java @@ -372,14 +372,9 @@ 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++) { - if (staticComparison && keys[comparisonIndex].kind() == PrimaryKey.Kind.STATIC) - inPartition = false; - assertCompareToAndEquals(key, keys[comparisonIndex], inPartition ? 0 : -1); + assertCompareToAndEquals(key, keys[comparisonIndex], -1); assertCompareToAndEquals(tokenOnlyKey, keys[comparisonIndex], tokenOnlyKey.token().equals(keys[comparisonIndex].token()) ? 0 : -1); } }