You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/07/23 16:41:14 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #735: Cassandra 16092: CEP-7 port Index Group for Storage Attached Index
adelapena commented on a change in pull request #735:
URL: https://github.com/apache/cassandra/pull/735#discussion_r675693985
##########
File path: src/java/org/apache/cassandra/service/reads/DataResolver.java
##########
@@ -121,22 +121,12 @@ private boolean needsReplicaFilteringProtection()
if (command.rowFilter().isEmpty())
return false;
- IndexMetadata indexMetadata = command.indexMetadata();
-
+ Index.QueryPlan queryPlan = command.indexQueryPlan();
+ IndexMetadata indexMetadata = queryPlan == null ? null : queryPlan.getFirst().getIndexMetadata();
Review comment:
I think we don't need getting the `IndexMetatada` anymore, IIRC those checks where here to avoid getting the `ColumnFamilyStore` and the `Index` if possible, but now the query plan has all we need.
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -364,6 +382,23 @@ default SSTableFlushObserver getFlushObserver(Descriptor descriptor, OperationTy
*/
public AbstractType<?> customExpressionValueType();
+ /**
+ * If the index supports custom search expressions using the
+ * {@code SELECT * FROM table WHERE expr(index_name, expression)} syntax, this method should return a new
+ * {@link RowFilter.CustomExpression} for the specified expression value. Index implementations may provide their
+ * own implementations using method {@link RowFilter.CustomExpression#isSatisfiedBy(TableMetadata, DecoratedKey, Row)}
+ * to filter reconciled rows in the coordinator. Otherwise, the default implementation will accept all rows.
+ * See DB-2185 and DSP-16537 for further details.
Review comment:
We shouldn't use references to Jira tickets that are not OSS
##########
File path: src/java/org/apache/cassandra/index/SingletonIndexGroup.java
##########
@@ -0,0 +1,112 @@
+/*
+ *
+ * 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;
+
+import java.util.Collections;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import org.apache.cassandra.db.DecoratedKey;
+import org.apache.cassandra.db.Memtable;
+import org.apache.cassandra.db.RegularAndStaticColumns;
+import org.apache.cassandra.db.WriteContext;
+import org.apache.cassandra.db.filter.RowFilter;
+import org.apache.cassandra.db.lifecycle.LifecycleNewTracker;
+import org.apache.cassandra.index.transactions.IndexTransaction;
+import org.apache.cassandra.io.sstable.Component;
+import org.apache.cassandra.io.sstable.Descriptor;
+import org.apache.cassandra.io.sstable.format.SSTableFlushObserver;
+import org.apache.cassandra.schema.TableMetadata;
+
+/**
+ * An {@link Index.Group} containing a single {@link Index}, to which it just delegates the calls.
+ */
+public class SingletonIndexGroup implements Index.Group
+{
+ private final Index delegate;
+ private final Set<Index> indexes;
+
+ protected SingletonIndexGroup(Index delegate)
+ {
+ this.delegate = delegate;
+ this.indexes = Collections.singleton(delegate);
+ }
+
+ @Override
+ public Set<Index> getIndexes()
+ {
+ return indexes;
+ }
+
+ public Index getIndex()
+ {
+ return delegate;
+ }
+
+ @Override
+ public void addIndex(Index index)
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void removeIndex(Index index)
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean containsIndex(Index index)
+ {
+ return index.equals(delegate);
+ }
+
+ @Override
+ public Index.Indexer indexerFor(Predicate<Index> indexSelector,
+ DecoratedKey key,
+ RegularAndStaticColumns columns,
+ int nowInSec,
+ WriteContext ctx,
+ IndexTransaction.Type transactionType,
+ Memtable memtable)
+ {
+ return indexSelector.test(delegate) ? delegate.indexerFor(key, columns, nowInSec, ctx, transactionType, memtable) : null;
Review comment:
Super nit: can we break this line?
```suggestion
return indexSelector.test(delegate)
? delegate.indexerFor(key, columns, nowInSec, ctx, transactionType, memtable)
: null;
```
##########
File path: src/java/org/apache/cassandra/db/ReadCommand.java
##########
@@ -977,8 +972,8 @@ public void serialize(ReadCommand command, DataOutputPlus out, int version) thro
ColumnFilter.serializer.serialize(command.columnFilter(), out, version);
RowFilter.serializer.serialize(command.rowFilter(), out, version);
DataLimits.serializer.serialize(command.limits(), out, version, command.metadata().comparator);
- if (null != command.index)
- IndexMetadata.serializer.serialize(command.index, out, version);
+ if (null != command.indexQueryPlan)
+ IndexMetadata.serializer.serialize(command.indexQueryPlan.getFirst().getIndexMetadata(), out, version);
Review comment:
Perhaps we could add a comment indicating that we are using the name of one of the indexes in the plan to identify the index group because we want to keep compatibility with nodes without groups, and that the plan can be different among nodes (which is a good thing)
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -364,6 +382,23 @@ default SSTableFlushObserver getFlushObserver(Descriptor descriptor, OperationTy
*/
public AbstractType<?> customExpressionValueType();
+ /**
+ * If the index supports custom search expressions using the
+ * {@code SELECT * FROM table WHERE expr(index_name, expression)} syntax, this method should return a new
+ * {@link RowFilter.CustomExpression} for the specified expression value. Index implementations may provide their
+ * own implementations using method {@link RowFilter.CustomExpression#isSatisfiedBy(TableMetadata, DecoratedKey, Row)}
+ * to filter reconciled rows in the coordinator. Otherwise, the default implementation will accept all rows.
+ * See DB-2185 and DSP-16537 for further details.
+ *
+ * @param metadata the indexed table metadata
+ * @param value the custom expression value
+ * @return a custom index expression for the specified value
+ */
+ default RowFilter.CustomExpression customExpressionFor(TableMetadata metadata, ByteBuffer value)
Review comment:
I think this method shouldn't be part of this ticket. IIRC we added it to allow indexes using custom expression to benefit from replica filtering protection (CASSANDRA-8272). However I think we are missing the part where `RowFilter.CustomExpression` uses it to delegate its instantiation to the index, the changes in `DataResolver` and the tests. IMO those changes should be done in a separate ticket, wdyt?
##########
File path: src/java/org/apache/cassandra/io/sstable/format/SSTableWriter.java
##########
@@ -174,20 +191,24 @@ public static SSTableWriter create(Descriptor descriptor,
// but the components are unmodifiable after construction
components.add(Component.CRC);
}
+
+ components.addAll(indexComponents);
+
return components;
}
private static Collection<SSTableFlushObserver> observers(Descriptor descriptor,
- Collection<Index> indexes,
- OperationType operationType)
+ Collection<Index.Group> indexGroups,
+ LifecycleNewTracker tracker,
Review comment:
Nit: the import for `OperationType` is not needed anymore.
##########
File path: src/java/org/apache/cassandra/db/ReadCommand.java
##########
@@ -436,15 +431,15 @@ public UnfilteredPartitionIterator executeLocally(ReadExecutionController execut
long startTimeNanos = System.nanoTime();
ColumnFamilyStore cfs = Keyspace.openAndGetStore(metadata());
- Index index = getIndex(cfs);
+ Index.QueryPlan indexQueryPlan = indexQueryPlan();
Index.Searcher searcher = null;
- if (index != null)
+ if (indexQueryPlan != null)
{
- if (!cfs.indexManager.isIndexQueryable(index))
- throw new IndexNotAvailableException(index);
+ cfs.indexManager.checkQueryability(indexQueryPlan);
- searcher = index.searcherFor(this);
+ Index index = indexQueryPlan.getFirst();
+ searcher = indexQueryPlan.searcherFor(this);
Tracing.trace("Executing read on {}.{} using index {}", cfs.metadata.keyspace, cfs.metadata.name, index.getIndexMetadata().name);
Review comment:
Maybe we can list the indexes, keeping the current format if the plan only has one index, to keep compatibility, for example with something like:
```java
Tracing.trace("Executing read on {}.{} using index{} {}",
cfs.metadata.keyspace,
cfs.metadata.name,
indexQueryPlan.getIndexes().size() == 1 ? "" : "es",
indexQueryPlan.getIndexes()
.stream()
.map(i -> i.getIndexMetadata().name)
.collect(Collectors.joining(",")));
```
--
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