blambov commented on code in PR #2065:
URL: https://github.com/apache/cassandra/pull/2065#discussion_r1067035265
##########
src/java/org/apache/cassandra/db/rows/UnfilteredRowIteratorWithLowerBound.java:
##########
@@ -185,70 +183,8 @@ public Row staticRow()
return super.staticRow();
}
- private static <V> ClusteringBound<V> createArtificialLowerBound(boolean
isReversed, ClusteringPrefix<V> from)
- {
- return from.accessor().factory().inclusiveOpen(isReversed,
from.getRawValues()).artificialLowerBound(isReversed);
- }
-
- /**
- * @return the lower bound stored on the index entry for this partition,
if available.
- */
- private ClusteringBound<?> getKeyCacheLowerBound()
- {
- // NOTE: CASSANDRA-11206 removed the lookup against the key-cache as
the IndexInfo objects are no longer
- // in memory for not heap backed IndexInfo objects (so, these are on
disk).
- // CASSANDRA-11369 is there to fix this afterwards.
-
- // Creating the iterator ensures that rowIndexEntry is loaded if
available (partitions bigger than
- // DatabaseDescriptor.column_index_size)
- if (!canUseMetadataLowerBound())
- maybeInit();
-
- RowIndexEntry rowIndexEntry =
sstable.getCachedPosition(partitionKey(), false);
Review Comment:
I think I have misled you here, checking the key cache still makes sense.
What I meant is that we do not want to initialize the iterator so that we
can check the key cache to decide that we did not want to initialize the
iterator. Also, calling `canUseMetadataLowerBound()` here is a bad idea in its
own right.
I would remove the lines above this in the method, but still try it first in
`lowerBound()`.
##########
src/java/org/apache/cassandra/db/Slice.java:
##########
@@ -106,6 +109,13 @@ public static Slice make(Clustering<?> start,
Clustering<?> end)
return new Slice(ClusteringBound.inclusiveStartOf(start),
ClusteringBound.inclusiveEndOf(end));
}
+ public static Slice make(ClusteringPrefix<?> start, ClusteringPrefix<?>
end)
+ {
+ // This doesn't give us what we want with the clustering prefix
+ assert start != Clustering.STATIC_CLUSTERING && end !=
Clustering.STATIC_CLUSTERING;
+ return make(start.asStartBound(), end.asEndBound());
Review Comment:
Could you add an inclusiveness comment to `asStartBound` and `asEndBound`
too?
##########
src/java/org/apache/cassandra/db/Slice.java:
##########
@@ -252,16 +253,8 @@ public Slice forPaging(ClusteringComparator comparator,
Clustering<?> lastReturn
*/
public boolean intersects(ClusteringComparator comparator, Slice other)
{
- // Empty slices never intersect anything (and we have to special case
it as there is many ways to build an
- // empty slice; for instance, without this, (0, 0) would intersect
Slice.ALL or [-1, 1]).
- if (isEmpty(comparator) || other.isEmpty(comparator))
- return false;
-
- // Otherwise, the slice intersects if it contains more than just their
boundaries. That is, the comparison
- // below needs to be strict, because for instance, a=[0, 3] and b=(3,
5] do not intersect, yet the end of `a`
- // is equal to end start of `b` as far as
`ClusteringPrefix.Kind#compare` goes (see the javadoc on that method
- // for why that is).
- return comparator.compare(start, other.end) < 0 &&
comparator.compare(end, other.start) > 0;
+ return comparator.compare(Comparables.max(start, other.start,
comparator),
Review Comment:
I think there should be a small comment why this works, e.g.
```
// Construct the intersection of the two slices and check if it is non-empty.
// This also works correctly when one or more of the inputs can be empty
(i.e. with end <= start).
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
