yifan-c commented on code in PR #1739:
URL: https://github.com/apache/cassandra/pull/1739#discussion_r930430933
##########
test/unit/org/apache/cassandra/cql3/validation/entities/WritetimeOrTTLTest.java:
##########
@@ -248,25 +1166,31 @@ private void assertInvalidPrimaryKeySelection(String
column) throws Throwable
{
assertInvalidThrowMessage("Cannot use selection function writetime on
PRIMARY KEY part " + column,
InvalidRequestException.class,
- String.format("SELECT WRITETIME(%s) FROM
%%s", column));
- assertInvalidThrowMessage("Cannot use selection function maxwritetime
on PRIMARY KEY part " + column,
- InvalidRequestException.class,
- String.format("SELECT MAXWRITETIME(%s) FROM
%%s", column));
+ format("SELECT WRITETIME(%s) FROM %%s",
column));
assertInvalidThrowMessage("Cannot use selection function ttl on
PRIMARY KEY part " + column,
InvalidRequestException.class,
- String.format("SELECT TTL(%s) FROM %%s",
column));
+ format("SELECT TTL(%s) FROM %%s", column));
+ }
+
+ private void assertInvalidListElementSelection(String column, String list)
throws Throwable
+ {
+ String message = format("Element selection is only allowed on sets and
maps, but %s is a list", list);
+ assertInvalidThrowMessage(message,
Review Comment:
add the assertion for `MAXWRITETIME` too.
##########
src/java/org/apache/cassandra/cql3/selection/ElementsSelector.java:
##########
@@ -43,12 +44,19 @@
*/
abstract class ElementsSelector extends Selector
{
+ /**
+ * An empty collection is composed of an int size of zero.
+ */
+ private static final ByteBuffer EMPTY_FROZEN_COLLECTION =
ByteBufferUtil.bytes(0);
+
protected final Selector selected;
+ protected final CollectionType<?> type;
Review Comment:
With the `type` defined in the abstract class, I think the one in the
implementation (line 276) can be removed.
##########
src/java/org/apache/cassandra/serializers/SetSerializer.java:
##########
@@ -248,4 +253,92 @@ public ByteBuffer getSliceFromSerialized(ByteBuffer
collection,
throw new MarshalException("Not enough bytes to read a set");
}
}
+
+ @Override
+ public int getIndexFromSerialized(ByteBuffer collection, ByteBuffer key,
AbstractType<?> comparator)
+ {
+ try
+ {
+ ByteBuffer input = collection.duplicate();
+ int n = readCollectionSize(input, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int offset = sizeOfCollectionSize(n, ProtocolVersion.V3);
+ for (int i = 0; i < n; i++)
+ {
+ ByteBuffer value = readValue(input,
ByteBufferAccessor.instance, offset, ProtocolVersion.V3);
+ offset += sizeOfValue(value, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int comparison = comparator.compareForCQL(value, key);
+ if (comparison == 0)
+ {
+ return i;
+ }
+ else if (comparison > 0)
+ {
+ // since the set is in sorted order, we know we've gone
too far and the element doesn't exist
+ return -1;
+ }
+ // else, we're before the element so continue
+ }
+ return -1;
+ }
+ catch (BufferUnderflowException e)
+ {
+ throw new MarshalException("Not enough bytes to read a set");
+ }
+ }
+
+ @Override
+ public Range<Integer> getIndexesRangeFromSerialized(ByteBuffer collection,
+ ByteBuffer from,
+ ByteBuffer to,
+ AbstractType<?>
comparator)
+ {
Review Comment:
add `from <= to` if both present
##########
src/java/org/apache/cassandra/serializers/MapSerializer.java:
##########
@@ -243,6 +247,96 @@ public ByteBuffer getSliceFromSerialized(ByteBuffer
collection,
}
}
+ @Override
+ public int getIndexFromSerialized(ByteBuffer collection, ByteBuffer key,
AbstractType<?> comparator)
+ {
+ try
+ {
+ ByteBuffer input = collection.duplicate();
+ int n = readCollectionSize(input, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int offset = sizeOfCollectionSize(n, ProtocolVersion.V3);
+ for (int i = 0; i < n; i++)
+ {
+ ByteBuffer kbb = readValue(input, ByteBufferAccessor.instance,
offset, ProtocolVersion.V3);
+ offset += sizeOfValue(kbb, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int comparison = comparator.compareForCQL(kbb, key);
+
+ if (comparison == 0)
+ return i;
+
+ if (comparison > 0)
+ // since the map is in sorted order, we know we've gone
too far and the element doesn't exist
+ return -1;
+
+ // comparison < 0
+ offset += skipValue(input, ByteBufferAccessor.instance,
offset, ProtocolVersion.V3);
+ }
+ return -1;
+ }
+ catch (BufferUnderflowException e)
+ {
+ throw new MarshalException("Not enough bytes to read a map");
+ }
+ }
+
+ @Override
+ public Range<Integer> getIndexesRangeFromSerialized(ByteBuffer collection,
+ ByteBuffer from,
+ ByteBuffer to,
+ AbstractType<?>
comparator)
+ {
Review Comment:
Add validation that `from <= to` if both are present. I tried to test with
`s[2..0]` the query passes, which is a bit surprising.
##########
test/unit/org/apache/cassandra/cql3/validation/entities/WritetimeOrTTLTest.java:
##########
@@ -248,25 +1166,31 @@ private void assertInvalidPrimaryKeySelection(String
column) throws Throwable
{
assertInvalidThrowMessage("Cannot use selection function writetime on
PRIMARY KEY part " + column,
InvalidRequestException.class,
- String.format("SELECT WRITETIME(%s) FROM
%%s", column));
- assertInvalidThrowMessage("Cannot use selection function maxwritetime
on PRIMARY KEY part " + column,
- InvalidRequestException.class,
- String.format("SELECT MAXWRITETIME(%s) FROM
%%s", column));
+ format("SELECT WRITETIME(%s) FROM %%s",
column));
Review Comment:
add the assertion for `MAXWRITETIME` too.
##########
test/unit/org/apache/cassandra/cql3/validation/entities/WritetimeOrTTLTest.java:
##########
@@ -248,25 +1166,31 @@ private void assertInvalidPrimaryKeySelection(String
column) throws Throwable
{
assertInvalidThrowMessage("Cannot use selection function writetime on
PRIMARY KEY part " + column,
InvalidRequestException.class,
- String.format("SELECT WRITETIME(%s) FROM
%%s", column));
- assertInvalidThrowMessage("Cannot use selection function maxwritetime
on PRIMARY KEY part " + column,
- InvalidRequestException.class,
- String.format("SELECT MAXWRITETIME(%s) FROM
%%s", column));
+ format("SELECT WRITETIME(%s) FROM %%s",
column));
assertInvalidThrowMessage("Cannot use selection function ttl on
PRIMARY KEY part " + column,
InvalidRequestException.class,
- String.format("SELECT TTL(%s) FROM %%s",
column));
+ format("SELECT TTL(%s) FROM %%s", column));
+ }
+
+ private void assertInvalidListElementSelection(String column, String list)
throws Throwable
+ {
+ String message = format("Element selection is only allowed on sets and
maps, but %s is a list", list);
+ assertInvalidThrowMessage(message,
+ InvalidRequestException.class,
+ format("SELECT WRITETIME(%s) FROM %%s",
column));
+ assertInvalidThrowMessage(message,
+ InvalidRequestException.class,
+ format("SELECT TTL(%s) FROM %%s", column));
}
- private void assertInvalidMultiCellSelection(String column, boolean
isCollection) throws Throwable
+ private void assertInvalidListSliceSelection(String column, String list)
throws Throwable
{
- String message = format("Cannot use selection function %%s on
non-frozen %s %s",
- isCollection ? "collection" : "UDT", column);
- assertInvalidThrowMessage(format(message, "writetime"),
+ String message = format("Slice selection is only allowed on sets and
maps, but %s is a list", list);
+ assertInvalidThrowMessage(message,
Review Comment:
also add the assertion for `MAXWRITETIME`
##########
src/java/org/apache/cassandra/cql3/selection/ElementsSelector.java:
##########
@@ -226,9 +234,19 @@ public ByteBuffer getOutput(ProtocolVersion
protocolVersion)
protected abstract ByteBuffer extractSelection(ByteBuffer collection);
- public void addInput(ProtocolVersion protocolVersion, InputRow input)
+ public void addInput(InputRow input)
+ {
+ selected.addInput(input);
+ }
+
+ protected int getElementIndex(ByteBuffer output, ByteBuffer key)
{
- selected.addInput(protocolVersion, input);
+ return type.getSerializer().getIndexFromSerialized(output, key,
keyType(type));
+ }
Review Comment:
This method is not used. It looks almost the same as the one in
`ElementsSelector.ElementSelector#getElementIndex` at line 305
##########
src/java/org/apache/cassandra/cql3/selection/ColumnTimestamps.java:
##########
@@ -0,0 +1,381 @@
+/*
+ * 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.selection;
+
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import com.google.common.collect.BoundType;
+import com.google.common.collect.Range;
+
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.db.rows.Cell;
+import org.apache.cassandra.serializers.CollectionSerializer;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+/**
+ * Represents a list of timestamps associated to a CQL column. Those
timestamps can either be writetimes or TTLs,
+ * according to {@link TimestampsType}.
+ */
+abstract class ColumnTimestamps
+{
+ /**
+ * The timestamps type.
+ */
+ protected final TimestampsType type;
+
+ protected ColumnTimestamps(TimestampsType type)
+ {
+ this.type = type;
+ }
+
+ /**
+ * @return the timestamps type
+ */
+ public TimestampsType type()
+ {
+ return type;
+ }
+
+ /**
+ * Retrieves the timestamps at the specified position.
+ *
+ * @param index the timestamps position
+ * @return the timestamps at the specified position or a {@link
#NO_TIMESTAMP}
+ */
+ public abstract ColumnTimestamps get(int index);
+
+ public abstract ColumnTimestamps max();
+
+ /**
+ * Returns a view of the portion of the timestamps within the specified
range.
+ *
+ * @param range the indexes range
+ * @return a view of the specified range within this {@link
ColumnTimestamps}
+ */
+ public abstract ColumnTimestamps slice(Range<Integer> range);
+
+ /**
+ * Converts the timestamps into their serialized form.
+ *
+ * @param protocolVersion the protocol version to use for the serialization
+ * @return the serialized timestamps
+ */
+ public abstract ByteBuffer toByteBuffer(ProtocolVersion protocolVersion);
+
+ /**
+ * Appends an empty timestamp at the end of this list.
+ */
+ public abstract void addNoTimestamp();
+
+ /**
+ * Appends the timestamp of the specified cell at the end of this list.
+ */
+ public abstract void addTimestampFrom(Cell<?> cell, int nowInSecond);
+
+ /**
+ * Creates a new {@link ColumnTimestamps} instance for the specified
column type.
+ *
+ * @param timestampType the timestamps type
+ * @param columnType the column type
+ * @return a {@link ColumnTimestamps} instance for the specified column
type
+ */
+ static ColumnTimestamps newTimestamps(TimestampsType timestampType,
AbstractType<?> columnType)
+ {
+ if (!columnType.isMultiCell())
+ return new SingleTimestamps(timestampType);
+
+ // For UserType we know that the size will not change, so we can
initialize the array with the proper capacity.
+ if (columnType instanceof UserType)
+ return new MultipleTimestamps(timestampType, ((UserType)
columnType).size());
+
+ return new MultipleTimestamps(timestampType, 0);
+ }
+
+ /**
+ * The type of represented timestamps.
+ */
+ public enum TimestampsType
+ {
+ WRITETIMES
+ {
+ @Override
+ long getTimestamp(Cell<?> cell, int nowInSecond)
+ {
+ return cell.timestamp();
+ }
+
+ @Override
+ long defaultValue()
+ {
+ return Long.MIN_VALUE;
+ }
+
+ @Override
+ ByteBuffer toByteBuffer(long timestamp)
+ {
+ return timestamp == defaultValue() ? null :
ByteBufferUtil.bytes(timestamp);
+ }
+ },
+ TTLS
+ {
+ @Override
+ long getTimestamp(Cell<?> cell, int nowInSecond)
+ {
+ if (!cell.isExpiring())
+ return defaultValue();
+
+ int remaining = cell.localDeletionTime() - nowInSecond;
+ return remaining >= 0 ? remaining : defaultValue();
+ }
+
+ @Override
+ long defaultValue()
+ {
+ return -1;
+ }
+
+ @Override
+ ByteBuffer toByteBuffer(long timestamp)
+ {
+ return timestamp == defaultValue() ? null :
ByteBufferUtil.bytes((int) timestamp);
+ }
+ };
+
+ /**
+ * Extracts the timestamp from the specified cell.
+ *
+ * @param cell the cell
+ * @param nowInSecond the query timestamp insecond
+ * @return the timestamp corresponding to this type
+ */
+ abstract long getTimestamp(Cell<?> cell, int nowInSecond);
+
+ /**
+ * Returns the value to use when there is no timestamp.
+ *
+ * @return the value to use when there is no timestamp
+ */
+ abstract long defaultValue();
+
+ /**
+ * Serializes the specified timestamp.
+ *
+ * @param timestamp the timestamp to serialize
+ * @return the bytes corresponding to the specified timestamp
+ */
+ abstract ByteBuffer toByteBuffer(long timestamp);
+ }
+
+ /**
+ * A {@link ColumnTimestamps} that doesn't contain any timestamps.
+ */
+ static final ColumnTimestamps NO_TIMESTAMP = new ColumnTimestamps(null)
+ {
+ @Override
+ public ColumnTimestamps get(int index)
+ {
+ return this;
+ }
+
+ @Override
+ public ColumnTimestamps max()
+ {
+ return this;
+ }
+
+ @Override
+ public ColumnTimestamps slice(Range<Integer> range)
+ {
+ return this;
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer(ProtocolVersion protocolVersion)
+ {
+ return null;
+ }
+
+ @Override
+ public void addNoTimestamp()
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void addTimestampFrom(Cell<?> cell, int nowInSecond)
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String toString()
+ {
+ return "no timestamp";
+ }
+ };
+
+ /**
+ * A {@link ColumnTimestamps} that can contains a single timestamp (for
columns that aren't multicell).
+ */
+ private static class SingleTimestamps extends ColumnTimestamps
+ {
+ protected long timestamp;
+
+ public SingleTimestamps(TimestampsType type)
+ {
+ this(type, type.defaultValue());
+ }
+
+ public SingleTimestamps(TimestampsType type, long timestamp)
+ {
+ super(type);
+ this.timestamp = timestamp;
+ }
+
+ @Override
+ public void addNoTimestamp()
+ {
+ timestamp = type.defaultValue();
+ }
+
+ @Override
+ public void addTimestampFrom(Cell<?> cell, int nowInSecond)
+ {
+ timestamp = type.getTimestamp(cell, nowInSecond);
+ }
+
+ @Override
+ public ColumnTimestamps get(int index)
+ {
+ // If this method is called it means that it is an element
selection on a frozen collection/UDT,
+ // so we can safely return this Timestamps as all the elements
also share that timestamp
+ return this;
+ }
+
+ @Override
+ public ColumnTimestamps max()
+ {
+ return this;
+ }
+
+ @Override
+ public ColumnTimestamps slice(Range<Integer> range)
+ {
+ return range.isEmpty() ? NO_TIMESTAMP : this;
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer(ProtocolVersion protocolVersion)
+ {
+ return timestamp == type.defaultValue() ? null :
type.toByteBuffer(timestamp);
+ }
+
+ @Override
+ public String toString()
+ {
+ return type + ": " + timestamp;
+ }
+ }
+
+ /**
+ * A {@link ColumnTimestamps} that can contain multiple timestamps (for
unfrozen collections or UDTs).
+ */
+ private static final class MultipleTimestamps extends ColumnTimestamps
+ {
+ private final List<Long> timestamps;
+
+ public MultipleTimestamps(TimestampsType type, int initialCapacity)
+ {
+ this(type, new ArrayList<>(initialCapacity));
+ }
+
+ public MultipleTimestamps(TimestampsType type, List<Long> timestamps)
+ {
+ super(type);
+ this.timestamps = timestamps;
+ }
+
+ @Override
+ public void addNoTimestamp()
+ {
+ timestamps.add(type.defaultValue());
+ }
+
+ @Override
+ public void addTimestampFrom(Cell<?> cell, int nowInSecond)
+ {
+ timestamps.add(type.getTimestamp(cell, nowInSecond));
+ }
+
+ @Override
+ public ColumnTimestamps get(int index)
+ {
+ return timestamps.isEmpty()
+ ? NO_TIMESTAMP
+ : new SingleTimestamps(type, timestamps.get(index));
+ }
+
+ @Override
+ public ColumnTimestamps max()
+ {
+ return timestamps.isEmpty()
+ ? NO_TIMESTAMP
+ : new SingleTimestamps(type, Collections.max(timestamps));
+ }
+
+ @Override
+ public ColumnTimestamps slice(Range<Integer> range)
+ {
+ if (range.isEmpty())
+ return NO_TIMESTAMP;
+
+ int from = !range.hasLowerBound() ? 0
+ : range.lowerBoundType() ==
BoundType.CLOSED ? range.lowerEndpoint()
+
: range.lowerEndpoint() + 1;
+ int to = !range.hasUpperBound() ? timestamps.size()
+ : range.upperBoundType() ==
BoundType.CLOSED ? range.upperEndpoint() + 1
+
: range.upperEndpoint();
Review Comment:
nit: just a different style to avoid the nested ternary operators.
Can you also add comments to explain calculations for `from` and `to`?
```suggestion
int from = 0;
int to = timestamps.size();
if (range.hasLowerBound())
{
from = range.lowerBoundType() == BoundType.CLOSED
? range.lowerEndpoint()
: range.lowerEndpoint() + 1;
}
if (range.hasUpperBound())
{
to = range.upperBoundType() == BoundType.CLOSED
? range.upperEndpoint() + 1
: range.upperEndpoint();
}
```
##########
src/java/org/apache/cassandra/serializers/MapSerializer.java:
##########
@@ -243,6 +247,96 @@ public ByteBuffer getSliceFromSerialized(ByteBuffer
collection,
}
}
+ @Override
+ public int getIndexFromSerialized(ByteBuffer collection, ByteBuffer key,
AbstractType<?> comparator)
+ {
+ try
+ {
+ ByteBuffer input = collection.duplicate();
+ int n = readCollectionSize(input, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int offset = sizeOfCollectionSize(n, ProtocolVersion.V3);
+ for (int i = 0; i < n; i++)
+ {
+ ByteBuffer kbb = readValue(input, ByteBufferAccessor.instance,
offset, ProtocolVersion.V3);
+ offset += sizeOfValue(kbb, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ int comparison = comparator.compareForCQL(kbb, key);
+
+ if (comparison == 0)
+ return i;
+
+ if (comparison > 0)
+ // since the map is in sorted order, we know we've gone
too far and the element doesn't exist
+ return -1;
+
+ // comparison < 0
+ offset += skipValue(input, ByteBufferAccessor.instance,
offset, ProtocolVersion.V3);
+ }
+ return -1;
+ }
+ catch (BufferUnderflowException e)
+ {
+ throw new MarshalException("Not enough bytes to read a map");
+ }
+ }
+
+ @Override
+ public Range<Integer> getIndexesRangeFromSerialized(ByteBuffer collection,
+ ByteBuffer from,
+ ByteBuffer to,
+ AbstractType<?>
comparator)
+ {
+ if (from == ByteBufferUtil.UNSET_BYTE_BUFFER && to ==
ByteBufferUtil.UNSET_BYTE_BUFFER)
+ return Range.closed(0, Integer.MAX_VALUE);
+
+ try
+ {
+ ByteBuffer input = collection.duplicate();
+ int n = readCollectionSize(input, ByteBufferAccessor.instance,
ProtocolVersion.V3);
+ input.position(input.position() + sizeOfCollectionSize(n,
ProtocolVersion.V3));
+ int start = from == ByteBufferUtil.UNSET_BYTE_BUFFER ? 0 : -1;
+ int end = to == ByteBufferUtil.UNSET_BYTE_BUFFER ? n : -1;
+
+ for (int i = 0; i < n; i++)
+ {
+ if (start >= 0 && end >= 0)
+ break;
+ else if (i > 0)
+ skipValue(input, ProtocolVersion.V3);
Review Comment:
The logic is the same as the one in `SetSerializer` except those 2 lines.
Maybe have a helper that combines both and adds a `valueSkipper` function, e.g.
`Consumer<Input>`?
--
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]
