Updated Branches: refs/heads/trunk 88f65a182 -> 54efc08d8
Fix querying with an empty (impossible) range patch by slebresne; reviewed by iamaleksey for CASSANDRA-5573 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/0beab74d Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/0beab74d Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/0beab74d Branch: refs/heads/trunk Commit: 0beab74df98cf4022717a686ea087e546e67dbf8 Parents: 6e81f81 Author: Sylvain Lebresne <[email protected]> Authored: Tue Jul 9 16:05:29 2013 +0200 Committer: Sylvain Lebresne <[email protected]> Committed: Fri Jul 12 17:03:05 2013 +0200 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../apache/cassandra/cql3/QueryProcessor.java | 14 --- .../cql3/statements/SelectStatement.java | 104 +++++++++++++------ .../apache/cassandra/db/filter/ColumnSlice.java | 45 ++++---- 4 files changed, 99 insertions(+), 65 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/0beab74d/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 50a9075..0253d13 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -13,6 +13,7 @@ * cqlsh: fix handling of semicolons inside BATCH queries (CASSANDRA-5697) * Expose native protocol server status in nodetool info (CASSANDRA-5735) * Fix pathetic performance of range tombstones (CASSANDRA-5677) + * Fix querying with an empty (impossible) range (CASSANDRA-5573) 1.2.6 http://git-wip-us.apache.org/repos/asf/cassandra/blob/0beab74d/src/java/org/apache/cassandra/cql3/QueryProcessor.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/QueryProcessor.java b/src/java/org/apache/cassandra/cql3/QueryProcessor.java index 5ad6e77..fa019c0 100644 --- a/src/java/org/apache/cassandra/cql3/QueryProcessor.java +++ b/src/java/org/apache/cassandra/cql3/QueryProcessor.java @@ -94,20 +94,6 @@ public class QueryProcessor } } - public static void validateSliceFilter(CFMetaData metadata, SliceQueryFilter range) - throws InvalidRequestException - { - try - { - AbstractType<?> comparator = metadata.getComparatorFor(null); - ColumnSlice.validate(range.slices, comparator, range.reversed); - } - catch (IllegalArgumentException e) - { - throw new InvalidRequestException(e.getMessage()); - } - } - private static ResultMessage processStatement(CQLStatement statement, ConsistencyLevel cl, QueryState queryState, List<ByteBuffer> variables) throws RequestExecutionException, RequestValidationException { http://git-wip-us.apache.org/repos/asf/cassandra/blob/0beab74d/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 5db58ad..6224af6 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -121,9 +121,17 @@ public class SelectStatement implements CQLStatement cl.validateForRead(keyspace()); - List<Row> rows = isKeyRange - ? StorageProxy.getRangeSlice(getRangeCommand(variables), cl) - : StorageProxy.read(getSliceCommands(variables), cl); + List<Row> rows; + if (isKeyRange) + { + RangeSliceCommand command = getRangeCommand(variables); + rows = command == null ? Collections.<Row>emptyList() : StorageProxy.getRangeSlice(command, cl); + } + else + { + List<ReadCommand> commands = getSliceCommands(variables); + rows = commands == null ? Collections.<Row>emptyList() : StorageProxy.read(commands, cl); + } return processResults(rows, variables); } @@ -149,11 +157,20 @@ public class SelectStatement implements CQLStatement { try { - List<Row> rows = isKeyRange - ? RangeSliceVerbHandler.executeLocally(getRangeCommand(Collections.<ByteBuffer>emptyList())) - : readLocally(keyspace(), getSliceCommands(Collections.<ByteBuffer>emptyList())); + List<ByteBuffer> variables = Collections.<ByteBuffer>emptyList(); + List<Row> rows; + if (isKeyRange) + { + RangeSliceCommand command = getRangeCommand(variables); + rows = command == null ? Collections.<Row>emptyList() : RangeSliceVerbHandler.executeLocally(command); + } + else + { + List<ReadCommand> commands = getSliceCommands(variables); + rows = commands == null ? Collections.<Row>emptyList() : readLocally(keyspace(), commands); + } - return processResults(rows, Collections.<ByteBuffer>emptyList()); + return processResults(rows, variables); } catch (ExecutionException e) { @@ -199,14 +216,18 @@ public class SelectStatement implements CQLStatement // Note that we should not share the slice filter amongst the command, due to SliceQueryFilter not // being immutable due to its columnCounter used by the lastCounted() method // (this is fairly ugly and we should change that but that's probably not a tiny refactor to do that cleanly) - commands.add(new SliceFromReadCommand(keyspace(), key, queryPath, (SliceQueryFilter)makeFilter(variables))); + SliceQueryFilter filter = (SliceQueryFilter)makeFilter(variables); + if (filter == null) + return null; + + commands.add(new SliceFromReadCommand(keyspace(), key, queryPath, filter)); } } // ...of a list of column names else { // ByNames commands can share the filter - IDiskAtomFilter filter = makeFilter(variables); + IDiskAtomFilter filter = makeFilter(variables); // names filter are never null for (ByteBuffer key: keys) { QueryProcessor.validateKey(key); @@ -219,14 +240,20 @@ public class SelectStatement implements CQLStatement private RangeSliceCommand getRangeCommand(List<ByteBuffer> variables) throws RequestValidationException { IDiskAtomFilter filter = makeFilter(variables); + if (filter == null) + return null; + List<IndexExpression> expressions = getIndexExpressions(variables); // The LIMIT provided by the user is the number of CQL row he wants returned. // We want to have getRangeSlice to count the number of columns, not the number of keys. - return new RangeSliceCommand(keyspace(), + AbstractBounds<RowPosition> keyBounds = getKeyBounds(variables); + return keyBounds == null + ? null + : new RangeSliceCommand(keyspace(), columnFamily(), null, filter, - getKeyBounds(variables), + keyBounds, expressions, getLimit(), true, @@ -236,16 +263,33 @@ public class SelectStatement implements CQLStatement private AbstractBounds<RowPosition> getKeyBounds(List<ByteBuffer> variables) throws InvalidRequestException { IPartitioner<?> p = StorageService.getPartitioner(); - AbstractBounds<RowPosition> bounds; if (onToken) { Token startToken = getTokenBound(Bound.START, variables, p); Token endToken = getTokenBound(Bound.END, variables, p); - RowPosition start = includeKeyBound(Bound.START) ? startToken.minKeyBound() : startToken.maxKeyBound(); - RowPosition end = includeKeyBound(Bound.END) ? endToken.maxKeyBound() : endToken.minKeyBound(); - bounds = new Range<RowPosition>(start, end); + boolean includeStart = includeKeyBound(Bound.START); + boolean includeEnd = includeKeyBound(Bound.END); + + /* + * If we ask SP.getRangeSlice() for (token(200), token(200)], it will happily return the whole ring. + * However, wrapping range doesn't really make sense for CQL, and we want to return an empty result + * in that case (CASSANDRA-5573). So special case to create a range that is guaranteed to be empty. + * + * In practice, we want to return an empty result set if either startToken > endToken, or both are + * equal but one of the bound is excluded (since [a, a] can contains something, but not (a, a], [a, a) + * or (a, a)). Note though that in the case where startToken or endToken is the minimum token, then + * this special case rule should not apply. + */ + int cmp = startToken.compareTo(endToken); + if (!startToken.isMinimum() && !endToken.isMinimum() && (cmp > 0 || (cmp == 0 && (!includeStart || !includeEnd)))) + return null; + + RowPosition start = includeStart ? startToken.minKeyBound() : startToken.maxKeyBound(); + RowPosition end = includeEnd ? endToken.maxKeyBound() : endToken.minKeyBound(); + + return new Range<RowPosition>(start, end); } else { @@ -255,26 +299,21 @@ public class SelectStatement implements CQLStatement RowPosition startKey = RowPosition.forKey(startKeyBytes, p); RowPosition finishKey = RowPosition.forKey(finishKeyBytes, p); if (startKey.compareTo(finishKey) > 0 && !finishKey.isMinimum(p)) - { - if (p.preservesOrder()) - throw new InvalidRequestException("Start key must sort before (or equal to) finish key in your partitioner!"); - else - throw new InvalidRequestException("Start key sorts after end key. This is not allowed; you probably should not specify end key at all under random partitioner"); - } + return null; + if (includeKeyBound(Bound.START)) { - bounds = includeKeyBound(Bound.END) - ? new Bounds<RowPosition>(startKey, finishKey) - : new IncludingExcludingBounds<RowPosition>(startKey, finishKey); + return includeKeyBound(Bound.END) + ? new Bounds<RowPosition>(startKey, finishKey) + : new IncludingExcludingBounds<RowPosition>(startKey, finishKey); } else { - bounds = includeKeyBound(Bound.END) - ? new Range<RowPosition>(startKey, finishKey) - : new ExcludingBounds<RowPosition>(startKey, finishKey); + return includeKeyBound(Bound.END) + ? new Range<RowPosition>(startKey, finishKey) + : new ExcludingBounds<RowPosition>(startKey, finishKey); } } - return bounds; } private IDiskAtomFilter makeFilter(List<ByteBuffer> variables) @@ -290,12 +329,15 @@ public class SelectStatement implements CQLStatement int toGroup = cfDef.isCompact ? -1 : cfDef.columns.size(); ColumnSlice slice = new ColumnSlice(getRequestedBound(Bound.START, variables), getRequestedBound(Bound.END, variables)); + + if (slice.isAlwaysEmpty(cfDef.cfm.comparator, isReversed)) + return null; + SliceQueryFilter filter = new SliceQueryFilter(new ColumnSlice[]{slice}, isReversed, getLimit(), toGroup, multiplier); - QueryProcessor.validateSliceFilter(cfDef.cfm, filter); return filter; } else @@ -1010,8 +1052,8 @@ public class SelectStatement implements CQLStatement if (stmt.onToken) throw new InvalidRequestException("The token() function must be applied to all partition key components or none of them"); - // Under a non order perserving partitioner, the only time not restricting a key part is allowed is if none are restricted - if (!partitioner.preservesOrder() && i > 0 && stmt.keyRestrictions[i-1] != null) + // The only time not restricting a key part is allowed is if none are restricted + if (i > 0 && stmt.keyRestrictions[i-1] != null) throw new InvalidRequestException(String.format("Partition key part %s must be restricted since preceding part is", cname)); stmt.isKeyRange = true; http://git-wip-us.apache.org/repos/asf/cassandra/blob/0beab74d/src/java/org/apache/cassandra/db/filter/ColumnSlice.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/filter/ColumnSlice.java b/src/java/org/apache/cassandra/db/filter/ColumnSlice.java index cdcdc17..208e181 100644 --- a/src/java/org/apache/cassandra/db/filter/ColumnSlice.java +++ b/src/java/org/apache/cassandra/db/filter/ColumnSlice.java @@ -53,23 +53,23 @@ public class ColumnSlice * * @throws IllegalArgumentException if the input slices are not valid. */ - public static void validate(ColumnSlice[] slices, AbstractType<?> comparator, boolean reversed) - { - for (int i = 0; i < slices.length; i++) - { - ColumnSlice slice = slices[i]; - validate(slice, comparator, reversed); - if (i > 0) - { - if (slices[i - 1].finish.remaining() == 0 || slice.start.remaining() == 0) - throw new IllegalArgumentException("Invalid column slices: slices must be sorted and non-overlapping"); - - int cmp = comparator.compare(slices[i -1].finish, slice.start); - if (reversed ? cmp <= 0 : cmp >= 0) - throw new IllegalArgumentException("Invalid column slices: slices must be sorted and non-overlapping"); - } - } - } + //public static void validate(ColumnSlice[] slices, AbstractType<?> comparator, boolean reversed) + //{ + // for (int i = 0; i < slices.length; i++) + // { + // ColumnSlice slice = slices[i]; + // validate(slice, comparator, reversed); + // if (i > 0) + // { + // if (slices[i - 1].finish.remaining() == 0 || slice.start.remaining() == 0) + // throw new IllegalArgumentException("Invalid column slices: slices must be sorted and non-overlapping"); + + // int cmp = comparator.compare(slices[i -1].finish, slice.start); + // if (reversed ? cmp <= 0 : cmp >= 0) + // throw new IllegalArgumentException("Invalid column slices: slices must be sorted and non-overlapping"); + // } + // } + //} /** * Validate a column slices. @@ -77,11 +77,16 @@ public class ColumnSlice * * @throws IllegalArgumentException if the slice is not valid. */ - public static void validate(ColumnSlice slice, AbstractType<?> comparator, boolean reversed) + //public static void validate(ColumnSlice slice, AbstractType<?> comparator, boolean reversed) + //{ + // if (slice.isAlwaysEmpty(comparator, reversed)) + // throw new IllegalArgumentException("Slice finish must come after start in traversal order"); + //} + + public boolean isAlwaysEmpty(AbstractType<?> comparator, boolean reversed) { Comparator<ByteBuffer> orderedComparator = reversed ? comparator.reverseComparator : comparator; - if (slice.start.remaining() > 0 && slice.finish.remaining() > 0 && orderedComparator.compare(slice.start, slice.finish) > 0) - throw new IllegalArgumentException("Slice finish must come after start in traversal order"); + return (start.remaining() > 0 && finish.remaining() > 0 && orderedComparator.compare(start, finish) > 0); } public boolean includes(Comparator<ByteBuffer> cmp, ByteBuffer name)
