You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maedhroz (via GitHub)" <gi...@apache.org> on 2025/01/23 23:08:54 UTC
[PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
maedhroz opened a new pull request, #3825:
URL: https://github.com/apache/cassandra/pull/3825
patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-20238
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1927909954
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java:
##########
@@ -229,14 +229,21 @@ protected static PrimaryKey nullSafeMin(PrimaryKey a, PrimaryKey b)
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)
{
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;
Review Comment:
Revert this block to see `testIntersectionWithStaticUpdate()` fail
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1927909279
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -95,7 +95,7 @@ protected PrimaryKey computeNext()
// 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)
Review Comment:
should be equivalent to pre-patch
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#issuecomment-2616928706
Committed as https://github.com/apache/cassandra/commit/8e5bdc6d4b1a3e5ffe0c930386153dc6408c0e4f
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #3825: CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys
URL: https://github.com/apache/cassandra/pull/3825
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1927910711
##########
src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java:
##########
@@ -395,25 +395,25 @@ public ByteSource asComparableBytes(ByteComparable.Version version)
}
@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);
Review Comment:
Using non-strict filtering here should fail `testIntersectionWithStaticOverlap()`
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1927908939
##########
src/java/org/apache/cassandra/index/sai/disk/v1/postings/PostingListRangeIterator.java:
##########
@@ -89,7 +89,7 @@ public PostingListRangeIterator(IndexIdentifier indexIdentifier,
@Override
protected void performSkipTo(PrimaryKey nextKey)
{
- if (skipToKey != null && skipToKey.compareTo(nextKey) > 0)
+ if (skipToKey != null && skipToKey.compareTo(nextKey, false) > 0)
Review Comment:
Note: This is functionally equivalent to what was here before, as the default `compareTo()` used to be "non-strict".
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1929178539
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java:
##########
@@ -229,14 +229,21 @@ protected static PrimaryKey nullSafeMin(PrimaryKey a, PrimaryKey b)
if (a == null) return b;
if (b == null) return a;
- return a.compareToStrict(b) > 0 ? b : a;
+ return a.compareTo(b) > 0 ? b : a;
Review Comment:
why is this one not `b, true`? That is the default but not following the pattern of this patch/file?
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIterator.java:
##########
@@ -229,14 +229,21 @@ protected static PrimaryKey nullSafeMin(PrimaryKey a, PrimaryKey b)
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)
{
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)
Review Comment:
are there any other cases to worry about here? this feels very specific...
```
enum Kind
{
TOKEN(false),
SKINNY(false),
WIDE(true),
STATIC(true);
```
i don't see these documented really so i have to rely on the boolean... but `org.apache.cassandra.index.sai.utils.PrimaryKey.Factory.TokenOnlyPrimaryKey#TokenOnlyPrimaryKey` doesn't mean the partition doesn't have a clustering in the schema... doesn't this just mean we have a token? Looking at the usage i see range expressions using this...
--
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
Re: [PR] CASSANDRA-20238 Correct the default behavior of compareTo() when comparing WIDE and STATIC PrimaryKeys [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3825:
URL: https://github.com/apache/cassandra/pull/3825#discussion_r1927912202
##########
src/java/org/apache/cassandra/index/sai/utils/PrimaryKey.java:
##########
@@ -395,25 +395,25 @@ public ByteSource asComparableBytes(ByteComparable.Version version)
}
@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);
Review Comment:
To see the root of the problem, see `PrimaryKeysReducer` in `TrieMemoryIndex`, where we try to add the WIDE key to `PrimaryKeys` w/ an existing STATIC key.
--
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