adelapena commented on code in PR #4249:
URL: https://github.com/apache/cassandra/pull/4249#discussion_r2230997291
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -777,4 +786,28 @@ public void waitUntilListeningUnchecked()
throw new RuntimeException(e);
}
}
+
+ /**
+ * Returns the endpoints for the given keyspace that are known to be alive
and have a connection whose
+ * messaging version is older than the given version. To be used for
example when we want to be sure a message
+ * can be serialized to all endpoints, according to their negotiated
version at connection time.
+ *
+ * @param keyspace a keyspace
+ * @param version a messaging version
+ * @return a set of alive endpoints in the given keyspace with messaging
version below the given version
+ */
+ public Set<InetAddressAndPort>
endpointsWithConnectionsOnVersionBelow(String keyspace, int version)
Review Comment:
Nit: the `keyspace` param is unused.
##########
src/java/org/apache/cassandra/db/filter/IndexHints.md:
##########
@@ -0,0 +1,134 @@
+<!---
+Copyright DataStax, Inc.
+
+Licensed 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.
+-->
Review Comment:
This should be licensed to the ASF now.
##########
src/java/org/apache/cassandra/db/filter/IndexHints.java:
##########
@@ -0,0 +1,663 @@
+/*
+ * 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.db.filter;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+
+import org.apache.cassandra.cql3.QualifiedName;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.exceptions.InvalidRequestException;
+import org.apache.cassandra.index.Index;
+import org.apache.cassandra.index.IndexRegistry;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.net.MessagingService;
+import org.apache.cassandra.schema.IndexMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.utils.FBUtilities;
+
+import static java.lang.String.format;
+
+/**
+ * User-provided directives about what indexes should be used by a {@code
SELECT} query.
+ * See {@code IndexHints.md} for further details.
+ */
+public class IndexHints
+{
+ public static final String CONFLICTING_INDEXES_ERROR = "Indexes cannot be
both included and excluded: ";
+ public static final String WRONG_KEYSPACE_ERROR = "Index %s is not in the
same keyspace as the queried table.";
+ public static final String MISSING_INDEX_ERROR = "Table %s doesn't have an
index named %s";
+ public static final String NON_INCLUDABLE_INDEXES_ERROR = "It's not
possible to use all the specified included indexes with this query.";
+ public static final String TOO_MANY_INDEXES_ERROR = format("Cannot have
more than %d included/excluded indexes, found ", Short.MAX_VALUE);
+
+ public static final IndexHints NONE = new
IndexHints(Collections.emptySet(), Collections.emptySet())
+ {
+ @Override
+ public boolean includes(Index index)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean includes(String indexName)
+ {
+ return false;
+ }
+
+ @Override
+ public Set<Index> includedIn(Collection<Index> indexes)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public boolean includesAnyOf(Collection<Index> indexes)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean excludes(Index index)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean excludes(String indexName)
+ {
+ return false;
+ }
+
+ @Override
+ public <T extends Index> Set<T> notExcluded(Iterable<T> indexes)
+ {
+ return Sets.newHashSet(indexes);
+ }
+
+ @Override
+ public void validate(@Nullable Index.QueryPlan queryPlan)
+ {
+ // nothing to validate
+ }
+
+ @Override
+ public Comparator<Index.QueryPlan> comparator()
+ {
+ // no index hints, so all plans are equal in that respect
+ return (x, y) -> 0;
+ }
+ };
+
+ public static final Serializer serializer = new Serializer();
+
+ /**
+ * The indexes to use when executing a query.
+ */
+ public final Set<IndexMetadata> included;
+
+ /**
+ * The indexes not to use when executing the query.
+ */
+ public final Set<IndexMetadata> excluded;
+
+ private IndexHints(Set<IndexMetadata> included, Set<IndexMetadata>
excluded)
+ {
+ this.included = included;
+ this.excluded = excluded;
+ }
+
+ /**
+ * @param index an index
+ * @return {@code true} if the index is included, {@code false} otherwise
+ */
+ public boolean includes(Index index)
+ {
+ return includes(index.getIndexMetadata().name);
+ }
+
+ /**
+ * @param indexName the name of an index
+ * @return {@code true} if the index is included, {@code false} otherwise
+ */
+ public boolean includes(String indexName)
+ {
+ for (IndexMetadata i : included)
+ {
+ if (i.name.equals(indexName))
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Returns the indexes in the specified collection of indexes that are
included by these hints.
+ *
+ * @param indexes a collection of indexes
+ * @return the indexes that are included by these hints
+ */
+ public Set<Index> includedIn(Collection<Index> indexes)
Review Comment:
Nit: unused method
##########
src/java/org/apache/cassandra/db/filter/IndexHints.java:
##########
@@ -0,0 +1,663 @@
+/*
+ * 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.db.filter;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+
+import org.apache.cassandra.cql3.QualifiedName;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.exceptions.InvalidRequestException;
+import org.apache.cassandra.index.Index;
+import org.apache.cassandra.index.IndexRegistry;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.net.MessagingService;
+import org.apache.cassandra.schema.IndexMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.utils.FBUtilities;
+
+import static java.lang.String.format;
+
+/**
+ * User-provided directives about what indexes should be used by a {@code
SELECT} query.
+ * See {@code IndexHints.md} for further details.
+ */
+public class IndexHints
+{
+ public static final String CONFLICTING_INDEXES_ERROR = "Indexes cannot be
both included and excluded: ";
+ public static final String WRONG_KEYSPACE_ERROR = "Index %s is not in the
same keyspace as the queried table.";
+ public static final String MISSING_INDEX_ERROR = "Table %s doesn't have an
index named %s";
+ public static final String NON_INCLUDABLE_INDEXES_ERROR = "It's not
possible to use all the specified included indexes with this query.";
+ public static final String TOO_MANY_INDEXES_ERROR = format("Cannot have
more than %d included/excluded indexes, found ", Short.MAX_VALUE);
+
+ public static final IndexHints NONE = new
IndexHints(Collections.emptySet(), Collections.emptySet())
+ {
+ @Override
+ public boolean includes(Index index)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean includes(String indexName)
+ {
+ return false;
+ }
+
+ @Override
+ public Set<Index> includedIn(Collection<Index> indexes)
+ {
+ return Collections.emptySet();
+ }
+
+ @Override
+ public boolean includesAnyOf(Collection<Index> indexes)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean excludes(Index index)
+ {
+ return false;
+ }
+
+ @Override
+ public boolean excludes(String indexName)
+ {
+ return false;
+ }
+
+ @Override
+ public <T extends Index> Set<T> notExcluded(Iterable<T> indexes)
+ {
+ return Sets.newHashSet(indexes);
+ }
+
+ @Override
+ public void validate(@Nullable Index.QueryPlan queryPlan)
+ {
+ // nothing to validate
+ }
+
+ @Override
+ public Comparator<Index.QueryPlan> comparator()
+ {
+ // no index hints, so all plans are equal in that respect
+ return (x, y) -> 0;
+ }
+ };
+
+ public static final Serializer serializer = new Serializer();
+
+ /**
+ * The indexes to use when executing a query.
+ */
+ public final Set<IndexMetadata> included;
+
+ /**
+ * The indexes not to use when executing the query.
+ */
+ public final Set<IndexMetadata> excluded;
+
+ private IndexHints(Set<IndexMetadata> included, Set<IndexMetadata>
excluded)
+ {
+ this.included = included;
+ this.excluded = excluded;
+ }
+
+ /**
+ * @param index an index
+ * @return {@code true} if the index is included, {@code false} otherwise
+ */
+ public boolean includes(Index index)
+ {
+ return includes(index.getIndexMetadata().name);
+ }
+
+ /**
+ * @param indexName the name of an index
+ * @return {@code true} if the index is included, {@code false} otherwise
+ */
+ public boolean includes(String indexName)
+ {
+ for (IndexMetadata i : included)
+ {
+ if (i.name.equals(indexName))
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Returns the indexes in the specified collection of indexes that are
included by these hints.
+ *
+ * @param indexes a collection of indexes
+ * @return the indexes that are included by these hints
+ */
+ public Set<Index> includedIn(Collection<Index> indexes)
+ {
+ Set<Index> result = new HashSet<>();
+ for (Index index : indexes)
+ {
+ if (includes(index))
+ result.add(index);
+ }
+ return result;
+ }
+
+ /**
+ * @param indexes a collection of indexes
+ * @return {@code true} if any of the indexes is included, {@code false}
otherwise
+ */
+ public boolean includesAnyOf(Collection<Index> indexes)
Review Comment:
Nit: unused method
##########
test/distributed/org/apache/cassandra/distributed/test/sai/IndexHintsDistributedTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.distributed.test.sai;
+
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import net.bytebuddy.implementation.MethodDelegation;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.ConsistencyLevel;
+import org.apache.cassandra.distributed.api.ICoordinator;
+import org.apache.cassandra.distributed.test.TestBaseImpl;
+import org.apache.cassandra.net.MessagingService;
+import org.apache.cassandra.net.MessagingService.Version;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.cassandra.distributed.api.Feature.GOSSIP;
+import static org.apache.cassandra.distributed.api.Feature.NATIVE_PROTOCOL;
+import static org.apache.cassandra.distributed.api.Feature.NETWORK;
+import static org.apache.cassandra.distributed.shared.AssertUtils.assertRows;
+import static org.apache.cassandra.distributed.shared.AssertUtils.row;
+
+/**
+ * Distributed tests for {@link org.apache.cassandra.db.filter.IndexHints}.
+ */
+public class IndexHintsDistributedTest extends TestBaseImpl
+{
+ private static final int NUM_REPLICAS = 2;
+ private static final int RF = 2;
+
+ @Test
+ public void testIndexHintsWithCurrentVersion() throws Throwable
+ {
+ try (Cluster cluster = init(Cluster.build(NUM_REPLICAS)
+ .withConfig(config ->
config.with(GOSSIP).with(NETWORK).set("storage_compatibility_mode", "NONE"))
+ .start(), RF))
+ {
+ // null indicates that the query should succeed
+ testSelectWithIndexHints(cluster, null);
+ }
+ }
+
+ @Test
+ public void testIndexHintsWithAllOldVersion() throws Throwable
+ {
+ try (Cluster cluster = init(Cluster.build(NUM_REPLICAS)
+ .withConfig(config ->
config.with(GOSSIP).with(NETWORK).set("storage_compatibility_mode",
"CASSANDRA_4"))
+ .start(), RF))
+ {
+ testSelectWithIndexHints(cluster, "Index hints are not supported
in clusters below 14.");
+ }
+ }
+
+ @Test
+ public void testIndexHintsWithMixedVersions() throws Throwable
+ {
+ try (Cluster cluster = init(Cluster.build(NUM_REPLICAS)
+
.withInstanceInitializer(BB::install)
+ .withConfig(config ->
config.with(GOSSIP).with(NETWORK).with(NATIVE_PROTOCOL).set("storage_compatibility_mode",
"NONE"))
+ .start(), RF))
+ {
+ testSelectWithIndexHints(cluster, "Index hints are not supported
in clusters below 14.");
+ }
+ }
+
+ private static void testSelectWithIndexHints(Cluster cluster, String
expectedErrorMessage) throws Throwable
+ {
+ // create a schema with various indexes in the same column, so we can
provide hints to select between them
+ cluster.schemaChange(withKeyspace("CREATE TABLE %s.t (k int PRIMARY
KEY, v text)"));
+ cluster.schemaChange(withKeyspace("CREATE INDEX legacy_idx ON
%s.t(v)"));
+ cluster.schemaChange(withKeyspace("CREATE CUSTOM INDEX
non_analyzed_sai_idx ON %s.t(v) USING 'StorageAttachedIndex'"));
Review Comment:
I guess this could be named just `sai_idx` until we introduce tokenizing
analyzers, although maybe leaving it this way could help us to update it later?
##########
src/java/org/apache/cassandra/cql3/restrictions/MergedRestriction.java:
##########
@@ -335,11 +349,11 @@ public void restrict(RangeSet<ClusteringElements>
rangeSet, QueryOptions options
}
@Override
- public void addToRowFilter(RowFilter filter, IndexRegistry indexRegistry,
QueryOptions options)
+ public void addToRowFilter(RowFilter filter, IndexRegistry indexRegistry,
QueryOptions options,IndexHints indexHints)
Review Comment:
```suggestion
public void addToRowFilter(RowFilter filter, IndexRegistry
indexRegistry, QueryOptions options, IndexHints indexHints)
```
##########
test/unit/org/apache/cassandra/cql3/CQLTester.java:
##########
@@ -252,6 +257,16 @@ public abstract class CQLTester
public static final String RACK1 = ServerTestUtils.RACK1;
protected static final int ASSERTION_TIMEOUT_SECONDS = 15;
+ /**
+ * Whether to use coorfinator execution in {@link #execute(String,
Object...)}, so queries get full validation and
Review Comment:
```suggestion
* Whether to use coordinator execution in {@link #execute(String,
Object...)}, so queries get full validation and
```
##########
src/java/org/apache/cassandra/index/SecondaryIndexManager.java:
##########
@@ -1224,6 +1226,15 @@ public void deletePartition(UnfilteredRowIterator
partition, long nowInSec)
* <p>
* The filtered set then sorted by selectivity, as reported by the Index
implementations' getEstimatedResultRows
* method.
+ * Once we have the filtered set of indexes, one is selected as the best
one according to the following rules:
+ * <ol>
+ * <li>An index included by the query's index hints is better than an
index not included by the hints.</li>
+ * <li>If it's a contains restriction, then a non-analyzed index is
better.</li>
Review Comment:
This should be removed until we add analyzers.
##########
test/unit/org/apache/cassandra/cql3/CQLTester.java:
##########
@@ -3624,4 +3770,75 @@ public int hashCode()
return java.util.Objects.hash(user, protocolVersion,
shouldUseEncryption, shouldUseCertificate);
}
}
+
+ protected PlanSelectionAssertion assertThatIndexQueryPlanFor(String query,
Object[]... expectedRows)
+ {
+ // First execute the query capturing warnings and check the query
results
+ disablePreparedReuseForTest();
+ ClientWarn.instance.captureWarnings();
+ assertRowsIgnoringOrder(execute(query), expectedRows);
+ List<String> warnings = ClientWarn.instance.getWarnings();
Review Comment:
Probably this and the two related methods (`warns` and `doesntWarn`) can be
removed.
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
