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 2022/05/10 16:53:29 UTC
[GitHub] [cassandra] adelapena commented on a diff in pull request #1584: CASSANDRA-17425: Add new CQL function maxWritetime
adelapena commented on code in PR #1584:
URL: https://github.com/apache/cassandra/pull/1584#discussion_r869448565
##########
src/java/org/apache/cassandra/cql3/selection/Selection.java:
##########
@@ -376,6 +376,12 @@ private static List<ByteBuffer> rowToJson(List<ByteBuffer> row,
*/
public boolean collectTimestamps();
+ /**
+ * Checks if one of the selectors collect maxTimestamps.
Review Comment:
```suggestion
* Checks if one of the selectors collects maxTimestamps.
```
##########
src/java/org/apache/cassandra/cql3/selection/Selectable.java:
##########
@@ -245,18 +271,20 @@ public Selector.Factory newSelectorFactory(TableMetadata table,
if (column.isPrimaryKeyColumn())
throw new InvalidRequestException(
String.format("Cannot use selection function %s on PRIMARY KEY part %s",
- isWritetime ? "writeTime" : "ttl",
+ kind.name,
column.name));
- if (column.type.isCollection())
+
+ // maxwritetime is allowed for collection
Review Comment:
```suggestion
// only maxwritetime is allowed for collections
```
##########
test/unit/org/apache/cassandra/cql3/validation/entities/TimestampTest.java:
##########
@@ -97,6 +97,42 @@ public void testTimestampTTL() throws Throwable
row(1, null, null));
}
+ @Test
+ public void testMaxTimestamp() throws Throwable
+ {
+ String myType = createType("CREATE TYPE %s (a int, b int)");
+ createTable("CREATE TABLE %s (k int PRIMARY KEY, a text, " +
+ "l list<int>, fl frozen<list<int>>," +
+ "s set<int>, fs frozen<set<int>>," +
+ "m map<int, text>, fm frozen<map<int, text>>," +
+ "t " + myType + ", ft frozen<" + myType + ">)");
+ execute("INSERT INTO %s (k, a, l, fl, s, fs, m, fm, t, ft) VALUES " +
Review Comment:
Before writing any data on the collections, we could insert a row with only the primary key and then verify that `maxwritetime` returns `null` for all the types.
##########
test/unit/org/apache/cassandra/cql3/validation/entities/TimestampTest.java:
##########
@@ -97,6 +97,42 @@ public void testTimestampTTL() throws Throwable
row(1, null, null));
}
+ @Test
+ public void testMaxTimestamp() throws Throwable
+ {
+ String myType = createType("CREATE TYPE %s (a int, b int)");
+ createTable("CREATE TABLE %s (k int PRIMARY KEY, a text, " +
+ "l list<int>, fl frozen<list<int>>," +
+ "s set<int>, fs frozen<set<int>>," +
+ "m map<int, text>, fm frozen<map<int, text>>," +
+ "t " + myType + ", ft frozen<" + myType + ">)");
+ execute("INSERT INTO %s (k, a, l, fl, s, fs, m, fm, t, ft) VALUES " +
+ "(1, 'test', [1], [2], {1}, {2}, {1 : 'a'}, {2 : 'b'}, {a : 1, b : 1 }, {a : 2, b : 2}) USING TIMESTAMP 1");
+ // update the list, so its maxWritetime should be more advanced than other columns
+ execute("UPDATE %s USING TIMESTAMP 2 SET l = l + [10] WHERE k = 1");
Review Comment:
This only exercises the selection among multiple values for the list. It could be useful to test all the multicell types, probably adding a separate update and timestamp per type.
##########
src/java/org/apache/cassandra/cql3/selection/SelectorFactories.java:
##########
@@ -46,6 +46,11 @@ final class SelectorFactories implements Iterable<Selector.Factory>
*/
private boolean containsWritetimeFactory;
+ /**
+ * <code>true</code> if one of the factory creates maxWritetime selectors.
Review Comment:
```suggestion
* {@code true} if one of the factories creates maxWritetime selectors.
```
##########
src/java/org/apache/cassandra/cql3/selection/WritetimeOrTTLSelector.java:
##########
@@ -45,29 +43,30 @@ protected Selector deserialize(DataInputPlus in, int version, TableMetadata meta
ByteBuffer columnName = ByteBufferUtil.readWithVIntLength(in);
ColumnMetadata column = metadata.getColumn(columnName);
int idx = in.readInt();
- boolean isWritetime = in.readBoolean();
- return new WritetimeOrTTLSelector(column, idx, isWritetime);
+ int ordinal = in.readByte();
Review Comment:
Don't know if this would require a protocol bump, since I think 4.1 is going to be released with the current `v6-beta`.
##########
src/java/org/apache/cassandra/cql3/selection/Selectable.java:
##########
@@ -245,18 +271,20 @@ public Selector.Factory newSelectorFactory(TableMetadata table,
if (column.isPrimaryKeyColumn())
throw new InvalidRequestException(
String.format("Cannot use selection function %s on PRIMARY KEY part %s",
- isWritetime ? "writeTime" : "ttl",
+ kind.name,
column.name));
- if (column.type.isCollection())
+
+ // maxwritetime is allowed for collection
+ if (column.type.isCollection() && !kind.allowedForMultiCell())
Review Comment:
Not sure why this error was originally thrown for frozen collections, nor why it wasn't thrown for non-frozen, multi-cell UDTs. Maybe it should be `column.type.isMultiCell()` instead of `column.type.isCollection()`?
##########
src/java/org/apache/cassandra/cql3/selection/SelectorFactories.java:
##########
@@ -165,6 +171,17 @@ public boolean containsWritetimeSelectorFactory()
return containsWritetimeFactory;
}
+ /**
+ * Checks if this <code>SelectorFactories</code> contains at least one factory for maxWritetime selectors.
Review Comment:
```suggestion
* Checks if this {@link SelectorFactories} contains at least one factory for maxWritetime selectors
```
##########
src/java/org/apache/cassandra/cql3/selection/SelectorFactories.java:
##########
@@ -165,6 +171,17 @@ public boolean containsWritetimeSelectorFactory()
return containsWritetimeFactory;
}
+ /**
+ * Checks if this <code>SelectorFactories</code> contains at least one factory for maxWritetime selectors.
+ *
+ * @return <code>true</code> if this <code>SelectorFactories</code> contains at least one factory for maxWritetime
+ * selectors, <code>false</code> otherwise.
Review Comment:
```suggestion
* <code>true</code> if this <code>SelectorFactories</code> contains at least one factory for maxWritetime
* selectors, <code>false</code> otherwise.
* @return {@link true} if this {@link SelectorFactories} contains at least one factory for maxWritetime
* selectors, {@link false} otherwise.
```
--
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