adelapena commented on code in PR #2935:
URL: https://github.com/apache/cassandra/pull/2935#discussion_r1446026427
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -1011,6 +1011,12 @@ public void close() throws Exception
@VisibleForTesting
public static class Serializer implements IVersionedSerializer<ReadCommand>
{
+ private static final int IS_DIGEST = 0x01;
+ private static final int IS_FOR_THRIFT = 0x02;
+ private static final int HAS_INDEX = 0x04;
+ private static final int ACCEPTS_TRIANSIENT = 0x08;
+ private static final int STRICT_FILTERING = 0x10;
Review Comment:
I'd add some JavaDoc describing the meaning of the flag.
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -1011,6 +1011,12 @@ public void close() throws Exception
@VisibleForTesting
public static class Serializer implements IVersionedSerializer<ReadCommand>
{
+ private static final int IS_DIGEST = 0x01;
+ private static final int IS_FOR_THRIFT = 0x02;
+ private static final int HAS_INDEX = 0x04;
+ private static final int ACCEPTS_TRIANSIENT = 0x08;
Review Comment:
Nit: typo
```suggestion
private static final int ACCEPTS_TRANSIENT = 0x08;
```
##########
src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java:
##########
@@ -779,7 +779,12 @@ public RowFilter getRowFilter(IndexRegistry indexRegistry,
QueryOptions options)
if (filterRestrictions.isEmpty())
return RowFilter.none();
- RowFilter filter = RowFilter.create();
+ ConsistencyLevel cl = options.getConsistency();
+ boolean isStrict = cl == ConsistencyLevel.ONE || cl ==
ConsistencyLevel.LOCAL_ONE || cl == ConsistencyLevel.NODE_LOCAL
Review Comment:
We have a mostly identical check to emit
`SelectStatement.TOPK_CONSISTENCY_LEVEL_WARNING`. Maybe we can put this
condition in a method in `ConsistencyLevel`, for example:
```java
private boolean isSingleReplica()
{
return this == ConsistencyLevel.ONE
|| this == ConsistencyLevel.LOCAL_ONE
|| this == ConsistencyLevel.NODE_LOCAL;
}
```
Not sure about the method name, though. Maybe we can use
`needsReconciliation` and negate the result?
```java
private boolean needsReconciliation()
{
return this != ConsistencyLevel.ONE
&& this != ConsistencyLevel.LOCAL_ONE
&& this != ConsistencyLevel.NODE_LOCAL;
}
```
##########
src/java/org/apache/cassandra/index/sai/plan/FilterTree.java:
##########
@@ -96,22 +100,56 @@ private boolean localSatisfiedBy(DecoratedKey key,
Unfiltered unfiltered, Row st
if (filter.getIndexTermType().isNonFrozenCollection())
{
Iterator<ByteBuffer> valueIterator =
filter.getIndexTermType().valuesOf(row, now);
- result = op.apply(result, collectionMatch(valueIterator,
filter));
+ result = localOperator.apply(result,
collectionMatch(valueIterator, filter));
}
else
{
ByteBuffer value = filter.getIndexTermType().valueOf(key,
row, now);
- result = op.apply(result, singletonMatch(value, filter));
+ result = localOperator.apply(result, singletonMatch(value,
filter));
}
// If the operation is an AND then exit early if we get a
single false
- if (op == BooleanOperator.AND && !result)
+ if (localOperator == BooleanOperator.AND && !result)
return false;
+
+ // If the operation is an OR then exit early if we get a
single true
Review Comment:
```suggestion
// If the operation is a OR then exit early if we get a
single true
```
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -171,33 +179,70 @@ public UnfilteredRowIterator queryStorage(PrimaryKey key,
ReadExecutionControlle
* the {@link SSTableIndex}s that will satisfy the expression.
* <p>
* Each (expression, SSTable indexes) pair is then passed to
- * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext)}
+ * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext, boolean)}
* to search the in-memory index associated with the expression and the
SSTable indexes, the results of
* which are unioned and returned.
* <p>
- * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext)}
- * are added to a {@link KeyRangeIntersectionIterator} and returned.
+ * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext, boolean)}
+ * are added to a {@link KeyRangeIntersectionIterator} and returned if
strict filtering is allowed.
+ * <p>
+ * If strict filtering is not allowed, indexes are split into two groups
according to the repaired status of their
+ * backing SSTables. Results from searches over the repaired group are
added to a
+ * {@link KeyRangeIntersectionIterator}, which is then added, along with
results from searches on the unrepaired
+ * set, to a top-level {@link KeyRangeUnionIterator}, and returned. This
is done to ensure that AND queries do not
Review Comment:
Maybe mention that the unrepaired set also includes the memtables?
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -171,33 +179,70 @@ public UnfilteredRowIterator queryStorage(PrimaryKey key,
ReadExecutionControlle
* the {@link SSTableIndex}s that will satisfy the expression.
* <p>
* Each (expression, SSTable indexes) pair is then passed to
- * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext)}
+ * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext, boolean)}
* to search the in-memory index associated with the expression and the
SSTable indexes, the results of
* which are unioned and returned.
* <p>
- * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext)}
- * are added to a {@link KeyRangeIntersectionIterator} and returned.
+ * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext, boolean)}
+ * are added to a {@link KeyRangeIntersectionIterator} and returned if
strict filtering is allowed.
+ * <p>
+ * If strict filtering is not allowed, indexes are split into two groups
according to the repaired status of their
+ * backing SSTables. Results from searches over the repaired group are
added to a
+ * {@link KeyRangeIntersectionIterator}, which is then added, along with
results from searches on the unrepaired
+ * set, to a top-level {@link KeyRangeUnionIterator}, and returned. This
is done to ensure that AND queries do not
+ * prematurely filter out matches on un-repaired partial updates.
Post-filtering must also take this into
+ * account. (see {@link FilterTree#isSatisfiedBy(DecoratedKey, Unfiltered,
Row)})
*/
public KeyRangeIterator.Builder
getIndexQueryResults(Collection<Expression> expressions)
{
// VSTODO move ANN out of expressions and into its own abstraction?
That will help get generic ORDER BY support
expressions = expressions.stream().filter(e -> e.getIndexOperator() !=
Expression.IndexOperator.ANN).collect(Collectors.toList());
- KeyRangeIterator.Builder builder =
KeyRangeIntersectionIterator.builder(expressions.size());
-
- QueryViewBuilder queryViewBuilder = new QueryViewBuilder(expressions,
mergeRange);
-
- QueryViewBuilder.QueryView queryView = queryViewBuilder.build();
+ KeyRangeIterator.Builder builder = command.rowFilter().isStrict()
+ ?
KeyRangeIntersectionIterator.builder(expressions.size())
+ :
KeyRangeUnionIterator.builder(expressions.size());
+ QueryViewBuilder.QueryView queryView = new
QueryViewBuilder(expressions, mergeRange).build();
try
{
maybeTriggerGuardrails(queryView);
- for (Pair<Expression, Collection<SSTableIndex>> queryViewPair :
queryView.view)
+ if (command.rowFilter().isStrict())
Review Comment:
I wonder if we should add a system property to disable this protection
mechanism optionally. That might be helpful for use cases where users know that
they aren't going to make partial updates.
##########
src/java/org/apache/cassandra/db/filter/RowFilter.java:
##########
@@ -68,19 +68,22 @@ public abstract class RowFilter implements
Iterable<RowFilter.Expression>
protected final List<Expression> expressions;
- protected RowFilter(List<Expression> expressions)
+ private final boolean isStrict;
Review Comment:
I'd add some JavaDoc describing the meaning of the flag, and possibly
mentioning this JIRA ticket.
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -171,33 +179,70 @@ public UnfilteredRowIterator queryStorage(PrimaryKey key,
ReadExecutionControlle
* the {@link SSTableIndex}s that will satisfy the expression.
* <p>
* Each (expression, SSTable indexes) pair is then passed to
- * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext)}
+ * {@link IndexSearchResultIterator#build(Expression, Collection,
AbstractBounds, QueryContext, boolean)}
* to search the in-memory index associated with the expression and the
SSTable indexes, the results of
* which are unioned and returned.
* <p>
- * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext)}
- * are added to a {@link KeyRangeIntersectionIterator} and returned.
+ * The results from each call to {@link
IndexSearchResultIterator#build(Expression, Collection, AbstractBounds,
QueryContext, boolean)}
+ * are added to a {@link KeyRangeIntersectionIterator} and returned if
strict filtering is allowed.
+ * <p>
+ * If strict filtering is not allowed, indexes are split into two groups
according to the repaired status of their
+ * backing SSTables. Results from searches over the repaired group are
added to a
+ * {@link KeyRangeIntersectionIterator}, which is then added, along with
results from searches on the unrepaired
+ * set, to a top-level {@link KeyRangeUnionIterator}, and returned. This
is done to ensure that AND queries do not
+ * prematurely filter out matches on un-repaired partial updates.
Post-filtering must also take this into
+ * account. (see {@link FilterTree#isSatisfiedBy(DecoratedKey, Unfiltered,
Row)})
*/
public KeyRangeIterator.Builder
getIndexQueryResults(Collection<Expression> expressions)
{
// VSTODO move ANN out of expressions and into its own abstraction?
That will help get generic ORDER BY support
expressions = expressions.stream().filter(e -> e.getIndexOperator() !=
Expression.IndexOperator.ANN).collect(Collectors.toList());
- KeyRangeIterator.Builder builder =
KeyRangeIntersectionIterator.builder(expressions.size());
-
- QueryViewBuilder queryViewBuilder = new QueryViewBuilder(expressions,
mergeRange);
-
- QueryViewBuilder.QueryView queryView = queryViewBuilder.build();
+ KeyRangeIterator.Builder builder = command.rowFilter().isStrict()
+ ?
KeyRangeIntersectionIterator.builder(expressions.size())
+ :
KeyRangeUnionIterator.builder(expressions.size());
+ QueryViewBuilder.QueryView queryView = new
QueryViewBuilder(expressions, mergeRange).build();
try
{
maybeTriggerGuardrails(queryView);
- for (Pair<Expression, Collection<SSTableIndex>> queryViewPair :
queryView.view)
+ if (command.rowFilter().isStrict())
{
- KeyRangeIterator indexIterator =
IndexSearchResultIterator.build(queryViewPair.left, queryViewPair.right,
mergeRange, queryContext);
-
- builder.add(indexIterator);
+ // If strict filtering is enabled, evaluate indexes for both
repaired and un-repaired SSTables together.
+ // This usually means we are making this local index query in
the context of a user query that reads
+ // from a single replica and thus can safely perform local
intersections.
+ for (Pair<Expression, Collection<SSTableIndex>> queryViewPair
: queryView.view)
+
builder.add(IndexSearchResultIterator.build(queryViewPair.left,
queryViewPair.right, mergeRange, queryContext, true));
+ }
+ else
+ {
+ KeyRangeIterator.Builder repairedBuilder =
KeyRangeIntersectionIterator.builder(expressions.size());
+
+ for (Pair<Expression, Collection<SSTableIndex>> queryViewPair
: queryView.view)
Review Comment:
Nit: add braces for the for loop
--
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]
