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 2024/01/19 10:40:17 UTC
[PR] CASSANDRA-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
maedhroz opened a new pull request, #3054:
URL: https://github.com/apache/cassandra/pull/3054
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-19278
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1468092518
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
+
if (nextKey == null || nextKey.compareTo(highestKey) > 0)
Review Comment:
`compareToStrict` looks good. Also thanks for adding a test that exposes that problem.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1466706005
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
It is not very obvious to me it always terminates. This `if / else if` has a theoretical path where it does *not* advance the range at all. Let's assume the first range returns a SKINNY key and the second range returns only STATIC keys then I guess the first range might get stuck because neither condition would be ever true.
Im also not sure if now this invariant of the for loop in line 75 still holds:
```
// Once this inner loop finishes normally, all iterators are guaranteed to be at the same value.
```
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1466954075
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
+
if (nextKey == null || nextKey.compareTo(highestKey) > 0)
Review Comment:
Would it break end-to-end, or would it just return a `STATIC` `PrimaryKey` from the intersection, which would potentially cause more post-filtering than we want in `ResultRetriever`? In any case, I think in the case where we have a `STATIC` `highestKey`, and `WIDE` `PrimaryKey` can be treated as higher, and we could probably make that change without changing any of the existing `compareTo()` behavior, which seems to open up a can of worms.
In short, I think we could fix this doing something like...
```
if (nextKey == null || nextKey.compareTo(highestKey) > 0 || (nextKey.compareTo(highestKey) == 0 && nextKey.kind() == WIDE && highestKey.kind() == STATIC))
```
...or just add a `compareToStrict()` to `PrimaryKey` that encapsulates this. We **DO** want to avoid a case where we post-filter a whole partition when 2 `WIDE` key iterators don't match anything from each other in the context of a match from a `STATIC` key iterator. Let me work on this a bit...
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467984306
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,8 +81,23 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
- if (nextKey == null || nextKey.compareTo(highestKey) > 0)
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
Review Comment:
Fixed in Piotr's version
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1466706005
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
It is not very obvious to me it always terminates. This `if / else if` has a theoretical path where it does *not* advance the range at all.
As I understood this fix, we don't want to advance a STATIC key until all non-static keys were advanced - is it correct? A static key matches the whole partition, so it must intersect with all the wide keys from another range, and can be advanced only *after* those were used and reported as the result. It is *probably* correct, but very, very non obvious and requires a lot of additional knowledge (like e.g. assuming there can be either only WIDE/STATIC or SKINNY but never mixed).
So maybe instead of doing such complex conditions (I am really staring at it for 30 minutes and trying to understand the logic, constantly switching between "Oh I get it" and "Oh no, I don't") could you split the loop into two loops - one going over non-static keys and trying to advance them, and then another one trying to advance the static ones (only if all non-static agree and the highestkey jumped over the static ones) ?
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467086089
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
+
if (nextKey == null || nextKey.compareTo(highestKey) > 0)
Review Comment:
I've added a new commit with a fix and further testing that should address this. Let me know what you think...
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459661987
##########
src/java/org/apache/cassandra/index/sai/disk/RowMapping.java:
##########
@@ -173,18 +166,15 @@ public void add(PrimaryKey key, long sstableRowId) throws InMemoryTrie.SpaceExha
maxSSTableRowId = Math.max(maxSSTableRowId, sstableRowId);
- // data is written in token sorted order
- if (minKey == null)
- minKey = key;
- maxKey = key;
+ updateMinMaxKeys(key);
count++;
}
/**
- * Returns the SSTable row Id for a {@link PrimaryKey}
+ * Returns the SSTable row ID for a {@link PrimaryKey}
*
* @param key the {@link PrimaryKey}
- * @return a valid SSTable row Id for the {@link PrimaryKey} or -1 if the {@link PrimaryKey} doesn't exist
+ * @return a valid SSTable row ID for the {@link PrimaryKey} or -1 if the {@link PrimaryKey} doesn't exist
Review Comment:
I'm OCD about using all-caps `ID` in comments :)
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1466756493
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
+
if (nextKey == null || nextKey.compareTo(highestKey) > 0)
Review Comment:
What if `highestKey` is STATIC? It may mess up the algorithm if there are other `WIDE` keys, because it would basically make this condition false, as STATIC key == any wide keys in the same partition. So it could break intersecting other wide keys within the partition.
I think this would manifest if we had 2 regular columns and intersect it with a static column.
Fortunately the intersection clause limit = 2 would not allow it, but a user can change it and allow more than 2 columns in an intersection.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467690905
##########
src/java/org/apache/cassandra/index/sai/disk/SSTableKeyTracker.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.disk;
+
+import org.apache.cassandra.index.sai.utils.PrimaryKey;
+
+public abstract class SSTableKeyTracker
+{
+ public PrimaryKey minKey;
Review Comment:
Looks like min/max keys are currently only used exclusively (i.e. if there are statics, static will be used, otherwise regular), usages in MemtableIndexFlusher and SegmentBuilder also show mutual exclusivity. Should this logic just be encapsulated in this class, or are there usages that I am missing?
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,8 +81,23 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
- if (nextKey == null || nextKey.compareTo(highestKey) > 0)
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == Kind.STATIC && nextKey.compareTo(highestKey) < 0)
Review Comment:
here, `nextKey.compareTo(highestKey)` , is the reasoning behind this that nextKey is guaranteed to be static, but highestKey can only be non-static? I can imagine situations where we have two partitions without rows, and only with clusterings. Won’t we trigger `==` in this case?
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,8 +81,23 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
- if (nextKey == null || nextKey.compareTo(highestKey) > 0)
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
Review Comment:
nit: "skip if we've" (`v` is missing)
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #3054: CASSANDRA-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections
URL: https://github.com/apache/cassandra/pull/3054
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1469371266
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -56,52 +62,82 @@ private KeyRangeIntersectionIterator(Builder.Statistics statistics, List<KeyRang
@Override
protected PrimaryKey computeNext()
{
- // Range iterator that has been advanced in the previous cycle of the outer loop.
- // Initially there hasn't been the previous cycle, so set to null.
- int alreadyAvanced = -1;
-
- // The highest primary key seen on any range iterator so far.
+ // Advance one iterator to the next key and remember the key as the highest seen so far.
// It can become null when we reach the end of the iterator.
- PrimaryKey highestKey = getCurrent();
+ // If there are both static and non-static keys being iterated here, we advacne a non-static one,
Review Comment:
Nit: advacne -> advance
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459669699
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
This is the core of the fix. Two problems:
1.) We were advancing streams of STATIC `PrimaryKey` objects from static column indexes before keys from the same partition from non-static column indexes could match. This was discovered by Harry on in-memory indexes.
2.) When we did skip the STATIC key streams, we could skip w/ a non-static `PrimaryKey` object, which would map to a posting row ID that was after the row ID of the static row, and so we could skip past the static `PrimaryKey` we wanted. (This affected on-disk indexes only.)
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459678442
##########
test/unit/org/apache/cassandra/index/sai/cql/StaticColumnIndexTest.java:
##########
@@ -49,7 +49,26 @@ public void staticIndexAndNonStaticIndex() throws Throwable
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"),
+ beforeAndAfterFlush(() -> assertRows(execute("SELECT pk, ck, val1, val2 FROM %s WHERE val1 = 20 AND val2 = 2000"),
row(1, 2, 20, 2000)));
}
+
+ @Test
+ public void staticAndNonStaticRangeIntersection() throws Throwable
Review Comment:
This is a concrete reproduction of the bug Harry found (see https://github.com/apache/cassandra/pull/3044)
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459671138
##########
test/unit/org/apache/cassandra/index/sai/cql/RandomIntersectionTest.java:
##########
@@ -55,23 +59,29 @@ public class RandomIntersectionTest extends SAIRandomizedTester
@Parameterized.Parameter(4)
public boolean v2Cardinality;
+ @Parameterized.Parameter(5)
+ public Mode mode;
+
@Parameterized.Parameters(name = "{0}")
public static List<Object[]> parameters()
{
List<Object[]> parameters = new LinkedList<>();
- parameters.add(new Object[]{ "Large partition restricted high high", true, true, true, true });
- parameters.add(new Object[]{ "Large partition restricted low low", true, true, false, false });
- parameters.add(new Object[]{ "Large partition restricted high low", true, true, true, false });
- parameters.add(new Object[]{ "Large partition unrestricted high high", false, true, true, true });
- parameters.add(new Object[]{ "Large partition unrestricted low low", false, true, false, false });
- parameters.add(new Object[]{ "Large partition unrestricted high low", false, true, true, false });
- parameters.add(new Object[]{ "Small partition restricted high high", true, false, true, true });
- parameters.add(new Object[]{ "Small partition restricted low low", true, false, false, false });
- parameters.add(new Object[]{ "Small partition restricted high low", true, false, true, false });
- parameters.add(new Object[]{ "Small partition unrestricted high high", false, false, true, true });
- parameters.add(new Object[]{ "Small partition unrestricted low low", false, false, false, false });
- parameters.add(new Object[]{ "Small partition unrestricted high low", false, false, true, false });
+ for (Mode mode : Mode.values())
+ {
+ parameters.add(new Object[] { "Large partition restricted, high, high, " + mode, true, true, true, true, mode });
+ parameters.add(new Object[] { "Large partition restricted, low, low, " + mode, true, true, false, false, mode });
+ parameters.add(new Object[] { "Large partition restricted, high, low, " + mode, true, true, true, false, mode });
+ parameters.add(new Object[] { "Large partition unrestricted, high, high, " + mode, false, true, true, true, mode });
+ parameters.add(new Object[] { "Large partition unrestricted, low, low, " + mode, false, true, false, false, mode });
+ parameters.add(new Object[] { "Large partition unrestricted, high, low, " + mode, false, true, true, false, mode });
+ parameters.add(new Object[] { "Small partition restricted, high, high, " + mode, true, false, true, true, mode });
+ parameters.add(new Object[] { "Small partition restricted, low, low, " + mode, true, false, false, false, mode });
+ parameters.add(new Object[] { "Small partition restricted, high, low, " + mode, true, false, true, false, mode });
+ parameters.add(new Object[] { "Small partition unrestricted, high, high, " + mode, false, false, true, true, mode });
+ parameters.add(new Object[] { "Small partition unrestricted, low, low, " + mode, false, false, false, false, mode });
+ parameters.add(new Object[] { "Small partition unrestricted, high, low, " + mode, false, false, true, false, mode });
Review Comment:
In short, the test now looks at `static AND static`, `regular AND regular`, and `regular AND static`.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1466706005
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
It is not very obvious to me it always terminates. This `if / else if` has a theoretical path where it does *not* advance the range at all.
As I understood this fix, we don't want to advance a STATIC key until all non-static keys were advanced - is it correct? A static key matches the whole partition, so it must intersect with all the wide keys from another range, and can be advanced only *after* those were used and reported as the result.
So maybe instead of doing such complex conditions (I am really staring at it for 30 minutes and trying to understand the logic, constantly switching between "Oh I get it" and "Oh no, I don't") could you split the loop into two loops - one going over non-static keys and trying to advance them, and then another one trying to advance the static ones (only if all non-static agree and the highestkey jumped over the static ones) ?
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#issuecomment-1915007240
Committed as https://github.com/apache/cassandra/commit/87f2a7b7637a346cdeea23e0b74d9968ccfca619
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459662806
##########
src/java/org/apache/cassandra/index/sai/disk/SSTableKeyTracker.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.disk;
+
+import org.apache.cassandra.index.sai.utils.PrimaryKey;
+
+public abstract class SSTableKeyTracker
+{
+ public PrimaryKey minKey;
+ public PrimaryKey maxKey;
+ public PrimaryKey minStaticKey;
+ public PrimaryKey maxStaticKey;
+
+ public long maxSSTableRowId = -1;
+
+ protected int count;
+
+ protected void updateMinMaxKeys(PrimaryKey key)
+ {
+ // Data is written in primary key order. If a schema contains clustering keys, it may also contain static
+ // columns. We track the min and max static keys separately here so that we can pass them to the segment
+ // metadata for indexes on static columns.
Review Comment:
I more or less created `SSTableKeyTracker` to avoid duplicating this comment and logic...
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459662806
##########
src/java/org/apache/cassandra/index/sai/disk/SSTableKeyTracker.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.disk;
+
+import org.apache.cassandra.index.sai.utils.PrimaryKey;
+
+public abstract class SSTableKeyTracker
+{
+ public PrimaryKey minKey;
+ public PrimaryKey maxKey;
+ public PrimaryKey minStaticKey;
+ public PrimaryKey maxStaticKey;
+
+ public long maxSSTableRowId = -1;
+
+ protected int count;
+
+ protected void updateMinMaxKeys(PrimaryKey key)
+ {
+ // Data is written in primary key order. If a schema contains clustering keys, it may also contain static
+ // columns. We track the min and max static keys separately here so that we can pass them to the segment
+ // metadata for indexes on static columns.
Review Comment:
I more or less created `SSTableKeyTracker` to avoid duplicating this comment and logic...
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1459889096
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
The thing I'm most worried about here is static/static intersections and whether they terminate in all cases.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1468094594
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,8 +81,23 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
- if (nextKey == null || nextKey.compareTo(highestKey) > 0)
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == Kind.STATIC && nextKey.compareTo(highestKey) < 0)
Review Comment:
Funny how I had exactly same question and realized later the if condition on line 88 would catch that case.
Yeah, it was a bit non-obvious, but looks correct. Anyway, the final version has fewer conditions. Take a look at that one.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467202036
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
While I wasn't quite able to settle on a two-loop solution that I liked, I've tried to a.) further document our assumptions, b.) create some reasonable guardrails (see the new intersection compatibility check in `buildIterator()` and `Kind#isIntersectable()`), and c.) expand the randomized tests even further.
I'm convinced, at this point, that the patch is correct, and that even a rewrite of this intersection logic at a later time would have sufficient tests ensuring it wouldn't make the same mistakes :)
Let me know what you think...
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "pkolaczk (via GitHub)" <gi...@apache.org>.
pkolaczk commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467555546
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,7 +77,19 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == PrimaryKey.Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == PrimaryKey.Kind.STATIC && nextKey.compareTo(highestKey) < 0)
+ // For a range of static keys, only skip if we'e advanced to a new partition, and when we
+ // do, skip to an actual static key. We may otherwise skip too far, as static row IDs always
+ // precede non-static ones in on-disk postings lists.
+ nextKey = nextOrNull(range, highestKey.toStatic());
Review Comment:
I am quite convinced your patch is correct, but I managed to find a way to make the conditions in the main loop simpler (and still leaving only one loop). Now there is exactly one condition to decide whether we bump up the iterator, and another one to decide how we advance (so we advance to the right partition in case of static column):
https://github.com/apache/cassandra/commit/b8b15e2a7c9c0a635ab40bcd9992476f0aa37c25
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1468015867
##########
src/java/org/apache/cassandra/index/sai/disk/SSTableKeyTracker.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.disk;
+
+import org.apache.cassandra.index.sai.utils.PrimaryKey;
+
+public abstract class SSTableKeyTracker
+{
+ public PrimaryKey minKey;
Review Comment:
You're right about `SegmentBuilder`. It was never necessary to worry about this there, as instances deal w/ single column indexes. `RowMapping` is global to an SSTable being flushed though, so I'm just going to localize it to that class and get rid of `SSTableKeyTracker`.
--
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-19278 Avoid over-skipping of key iterators from static column indexes during mixed intersections [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3054:
URL: https://github.com/apache/cassandra/pull/3054#discussion_r1467998872
##########
src/java/org/apache/cassandra/index/sai/iterators/KeyRangeIntersectionIterator.java:
##########
@@ -77,8 +81,23 @@ protected PrimaryKey computeNext()
if (index != alreadyAvanced)
{
KeyRangeIterator range = ranges.get(index);
- PrimaryKey nextKey = nextOrNull(range, highestKey);
- if (nextKey == null || nextKey.compareTo(highestKey) > 0)
+ PrimaryKey nextKey = range.getCurrent();
+
+ // Note that we will either have a data model that produces SKINNY primary keys or a data model
+ // that produces some combination of WIDE and STATIC prikary keys.
+ if (nextKey.kind() == Kind.WIDE || nextKey.kind() == highestKey.kind())
+ // We can always skip if the target is of the same kind or this range is non-static.
+ nextKey = nextOrNull(range, highestKey);
+ else if (nextKey.kind() == Kind.STATIC && nextKey.compareTo(highestKey) < 0)
Review Comment:
This has changed slightly w/ Piotr's version, but if the `highestKey` and `nextKey` had the same `kind()`, we would have hit the previous `if` condition on line 88.
--
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