Repository: cassandra Updated Branches: refs/heads/trunk d414c1cd8 -> a41b861fa
Catch empty/invalid bounds in SelectStatement Patch by Blake Eggleston; Reviewed by Aleksey Yeschenko for CASSANDRA-14849 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/a41b861f Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/a41b861f Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/a41b861f Branch: refs/heads/trunk Commit: a41b861fa4d4acfbcce13dd62b1e8f48be22f8ed Parents: d414c1c Author: Blake Eggleston <[email protected]> Authored: Thu Oct 25 13:12:02 2018 -0700 Committer: Blake Eggleston <[email protected]> Committed: Thu Dec 13 14:20:45 2018 -0800 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../cql3/statements/SelectStatement.java | 8 ++- src/java/org/apache/cassandra/db/Slice.java | 10 +++- src/java/org/apache/cassandra/db/Slices.java | 5 +- .../cql3/statements/SelectStatementTest.java | 63 ++++++++++++++++++++ .../apache/cassandra/db/filter/SliceTest.java | 33 ++++++++-- 6 files changed, 108 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 9b4ab59..bde5b52 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0 + * Catch empty/invalid bounds in SelectStatement (CASSANDRA-14849) * Auto-expand replication_factor for NetworkTopologyStrategy (CASSANDRA-14303) * Transient Replication: support EACH_QUORUM (CASSANDRA-14727) * BufferPool: allocating thread for new chunks should acquire directly (CASSANDRA-14832) http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index f847a6e..6e52ab1 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -20,6 +20,7 @@ package org.apache.cassandra.cql3.statements; import java.nio.ByteBuffer; import java.util.*; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import org.slf4j.Logger; @@ -625,7 +626,8 @@ public class SelectStatement implements CQLStatement return new ClusteringIndexNamesFilter(clusterings, isReversed); } - private Slices makeSlices(QueryOptions options) + @VisibleForTesting + public Slices makeSlices(QueryOptions options) throws InvalidRequestException { SortedSet<ClusteringBound> startBounds = restrictions.getClusteringColumnsBounds(Bound.START, options); @@ -637,7 +639,7 @@ public class SelectStatement implements CQLStatement { ClusteringBound start = startBounds.first(); ClusteringBound end = endBounds.first(); - return table.comparator.compare(start, end) > 0 + return Slice.isEmpty(table.comparator, start, end) ? Slices.NONE : Slices.with(table.comparator, Slice.make(start, end)); } @@ -651,7 +653,7 @@ public class SelectStatement implements CQLStatement ClusteringBound end = endIter.next(); // Ignore slices that are nonsensical - if (table.comparator.compare(start, end) > 0) + if (Slice.isEmpty(table.comparator, start, end)) continue; builder.add(start, end); http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/src/java/org/apache/cassandra/db/Slice.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/Slice.java b/src/java/org/apache/cassandra/db/Slice.java index 2517186..44f7ac3 100644 --- a/src/java/org/apache/cassandra/db/Slice.java +++ b/src/java/org/apache/cassandra/db/Slice.java @@ -159,7 +159,15 @@ public class Slice public static boolean isEmpty(ClusteringComparator comparator, ClusteringBound start, ClusteringBound end) { assert start.isStart() && end.isEnd(); - return comparator.compare(end, start) <= 0; + + int cmp = comparator.compare(start, end); + + if (cmp < 0) + return false; + else if (cmp > 0) + return true; + else + return start.isExclusive() || end.isExclusive(); } /** http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/src/java/org/apache/cassandra/db/Slices.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/Slices.java b/src/java/org/apache/cassandra/db/Slices.java index 9900112..9c7da79 100644 --- a/src/java/org/apache/cassandra/db/Slices.java +++ b/src/java/org/apache/cassandra/db/Slices.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.*; +import com.google.common.base.Preconditions; import com.google.common.collect.Iterators; import org.apache.cassandra.schema.ColumnMetadata; @@ -61,7 +62,7 @@ public abstract class Slices implements Iterable<Slice> if (slice.start() == ClusteringBound.BOTTOM && slice.end() == ClusteringBound.TOP) return Slices.ALL; - assert comparator.compare(slice.start(), slice.end()) <= 0; + Preconditions.checkArgument(!slice.isEmpty(comparator)); return new ArrayBackedSlices(comparator, new Slice[]{ slice }); } @@ -192,7 +193,7 @@ public abstract class Slices implements Iterable<Slice> public Builder add(Slice slice) { - assert comparator.compare(slice.start(), slice.end()) <= 0; + Preconditions.checkArgument(!slice.isEmpty(comparator)); if (slices.size() > 0 && comparator.compare(slices.get(slices.size()-1).end(), slice.start()) > 0) needsNormalizing = true; slices.add(slice); http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/test/unit/org/apache/cassandra/cql3/statements/SelectStatementTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/statements/SelectStatementTest.java b/test/unit/org/apache/cassandra/cql3/statements/SelectStatementTest.java new file mode 100644 index 0000000..5856bce --- /dev/null +++ b/test/unit/org/apache/cassandra/cql3/statements/SelectStatementTest.java @@ -0,0 +1,63 @@ +/* + * 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.cql3.statements; + +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.SchemaLoader; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.cql3.CQLStatement; +import org.apache.cassandra.cql3.QueryOptions; +import org.apache.cassandra.cql3.QueryProcessor; +import org.apache.cassandra.db.Slices; +import org.apache.cassandra.schema.KeyspaceParams; +import org.apache.cassandra.service.ClientState; + +public class SelectStatementTest +{ + + private static final String KEYSPACE = "ks"; + + @BeforeClass + public static void setupClass() + { + DatabaseDescriptor.daemonInitialization(); + SchemaLoader.prepareServer(); + SchemaLoader.createKeyspace(KEYSPACE, KeyspaceParams.simple(1)); + } + + private static SelectStatement parseSelect(String query) + { + CQLStatement stmt = QueryProcessor.parseStatement(query).prepare(ClientState.forInternalCalls()); + assert stmt instanceof SelectStatement; + return (SelectStatement) stmt; + } + + @Test + public void testNonsensicalBounds() + { + QueryProcessor.executeOnceInternal("CREATE TABLE ks.tbl (k int, c int, v int, primary key (k, c))"); + QueryProcessor.executeOnceInternal("INSERT INTO ks.tbl (k, c, v) VALUES (100, 10, 0)"); + Assert.assertEquals(Slices.NONE, parseSelect("SELECT * FROM ks.tbl WHERE k=100 AND c > 10 AND c <= 10").makeSlices(QueryOptions.DEFAULT)); + Assert.assertEquals(Slices.NONE, parseSelect("SELECT * FROM ks.tbl WHERE k=100 AND c < 10 AND c >= 10").makeSlices(QueryOptions.DEFAULT)); + Assert.assertEquals(Slices.NONE, parseSelect("SELECT * FROM ks.tbl WHERE k=100 AND c < 10 AND c > 10").makeSlices(QueryOptions.DEFAULT)); + } +} http://git-wip-us.apache.org/repos/asf/cassandra/blob/a41b861f/test/unit/org/apache/cassandra/db/filter/SliceTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/filter/SliceTest.java b/test/unit/org/apache/cassandra/db/filter/SliceTest.java index 9188c94..6c04500 100644 --- a/test/unit/org/apache/cassandra/db/filter/SliceTest.java +++ b/test/unit/org/apache/cassandra/db/filter/SliceTest.java @@ -30,6 +30,7 @@ import org.apache.cassandra.db.marshal.AbstractType; import org.apache.cassandra.db.marshal.Int32Type; import org.apache.cassandra.utils.ByteBufferUtil; +import static org.apache.cassandra.db.ClusteringPrefix.Kind.*; import static org.junit.Assert.*; public class SliceTest @@ -43,8 +44,8 @@ public class SliceTest types.add(Int32Type.instance); ClusteringComparator cc = new ClusteringComparator(types); - ClusteringPrefix.Kind sk = ClusteringPrefix.Kind.INCL_START_BOUND; - ClusteringPrefix.Kind ek = ClusteringPrefix.Kind.INCL_END_BOUND; + ClusteringPrefix.Kind sk = INCL_START_BOUND; + ClusteringPrefix.Kind ek = INCL_END_BOUND; // filter falls entirely before sstable Slice slice = Slice.make(makeBound(sk, 0, 0, 0), makeBound(ek, 1, 0, 0)); @@ -274,8 +275,8 @@ public class SliceTest types.add(Int32Type.instance); ClusteringComparator cc = new ClusteringComparator(types); - ClusteringPrefix.Kind sk = ClusteringPrefix.Kind.INCL_START_BOUND; - ClusteringPrefix.Kind ek = ClusteringPrefix.Kind.INCL_END_BOUND; + ClusteringPrefix.Kind sk = INCL_START_BOUND; + ClusteringPrefix.Kind ek = INCL_END_BOUND; // slice does intersect Slice slice = Slice.make(makeBound(sk), makeBound(ek)); @@ -323,6 +324,26 @@ public class SliceTest assertSlicesNormalization(cc, slices(s(-1, 2), s(-1, 3), s(5, 9)), slices(s(-1, 3), s(5, 9))); } + @Test + public void testIsEmpty() + { + List<AbstractType<?>> types = new ArrayList<>(); + types.add(Int32Type.instance); + types.add(Int32Type.instance); + ClusteringComparator cc = new ClusteringComparator(types); + + assertFalse(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5, 0), makeBound(INCL_END_BOUND, 5, 0))); + assertFalse(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5, 0), makeBound(EXCL_END_BOUND, 5, 1))); + assertFalse(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5), makeBound(EXCL_END_BOUND, 5, 1))); + + assertTrue(Slice.isEmpty(cc, makeBound(EXCL_START_BOUND, 5), makeBound(EXCL_END_BOUND, 5))); + assertTrue(Slice.isEmpty(cc, makeBound(EXCL_START_BOUND, 5), makeBound(EXCL_END_BOUND, 5, 1))); + assertTrue(Slice.isEmpty(cc, makeBound(EXCL_START_BOUND, 5, 1), makeBound(EXCL_END_BOUND, 5, 1))); + assertTrue(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5, 0), makeBound(INCL_END_BOUND, 4, 0))); + assertTrue(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5, 0), makeBound(EXCL_END_BOUND, 5))); + assertTrue(Slice.isEmpty(cc, makeBound(INCL_START_BOUND, 5, 0), makeBound(EXCL_END_BOUND, 3, 0))); + } + private static ClusteringBound makeBound(ClusteringPrefix.Kind kind, Integer... components) { ByteBuffer[] values = new ByteBuffer[components.length]; @@ -343,8 +364,8 @@ public class SliceTest private static Slice s(int start, int finish) { - return Slice.make(makeBound(ClusteringPrefix.Kind.INCL_START_BOUND, start), - makeBound(ClusteringPrefix.Kind.INCL_END_BOUND, finish)); + return Slice.make(makeBound(INCL_START_BOUND, start), + makeBound(INCL_END_BOUND, finish)); } private Slice[] slices(Slice... slices) --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
