ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1607060819
##########
test/unit/org/apache/cassandra/cql3/restrictions/ClusteringElementsTest.java:
##########
@@ -588,6 +589,50 @@ public void testExtend()
assertUnsupported("Range endpoints cannot be extended", () ->
bottom.extend(third));
}
+ @Test
+ public void testForCQLComparator()
+ {
+ {
Review Comment:
The brackets on lines 595 and 633 are not needed. We will need to shift to
the left all lines after removing those brackets.
##########
test/unit/org/apache/cassandra/cql3/validation/operations/DeleteTest.java:
##########
@@ -1152,10 +1186,16 @@ public void testDeleteAndReverseQueries() throws
Throwable
flush();
- execute("DELETE FROM %s WHERE k = ? AND i >= ? AND i <= ?", "a", 2, 7);
+ execute("DELETE FROM %s WHERE k = ? AND i >= ? AND i <= ?", "a", 2, 5);
assertRows(execute("SELECT i FROM %s WHERE k = ? ORDER BY i DESC",
"a"),
- row(9), row(8), row(1), row(0)
+ row(9), row(8), row(7), row(6), row(1), row(0)
+ );
+
Review Comment:
```suggestion
flush();
```
##########
test/unit/org/apache/cassandra/cql3/validation/operations/DeleteTest.java:
##########
@@ -75,7 +75,15 @@ private void testDeleteRange(boolean flushData, boolean
flushTombstone) throws T
execute("INSERT INTO %s (a, b, c) VALUES (?, ?, ?)", 2, 3, 4);
flush(flushData);
- execute("DELETE FROM %s WHERE a = ? AND b >= ?", 2, 2);
+ execute("DELETE FROM %s WHERE a = ? AND b >= ?", 2, 3);
Review Comment:
The test updates in this test class look good but I would probably also add
something to `testDeleteWithNoClusteringColumns`.
You could also update others like `testDeleteWithOneClusteringColumns`,
`testDeleteWithTwoClusteringColumns`, `testDeleteWithNonoverlappingRange`,
etc. Any test applicable to ranges? I do not expect big surprises, but it is
good against regressions, etc.
##########
src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java:
##########
@@ -470,4 +424,80 @@ public ClusteringBound<?> toBound(boolean isStart, boolean
isInclusive)
return isEmpty() ? BufferClusteringBound.TOP :
super.toBound(isStart, isInclusive);
}
}
+
+ public static class ClusteringElementsComparator implements
Comparator<ClusteringElements>
+ {
+ boolean forCQL;
+
+ public ClusteringElementsComparator()
+ {
+ this.forCQL = false;
+ }
+
+ public ClusteringElementsComparator(boolean forCQL)
+ {
+ this.forCQL = forCQL;
+ }
+
+ public int compare(ClusteringElements a, ClusteringElements b)
Review Comment:
```suggestion
@Override
public int compare(ClusteringElements a, ClusteringElements b)
```
##########
pylib/cqlshlib/test/test_cqlsh_completion.py:
##########
@@ -390,17 +390,17 @@ def test_complete_in_update(self):
self.trycompletions("UPDATE empty_table SET lonelycol = 'eggs' WHERE
TOKEN(lonelykey ",
choices=[',', ')'])
self.trycompletions("UPDATE empty_table SET lonelycol = 'eggs' WHERE
TOKEN(lonelykey) ",
- choices=['=', '<=', '>=', '<', '>'])
+ choices=['=', '<=', '>=', 'BETWEEN', '<', '>'])
Review Comment:
We do not support BETWEEN in `UPDATE`?
##########
test/unit/org/apache/cassandra/cql3/validation/operations/SelectMultiColumnRelationTest.java:
##########
@@ -38,10 +38,16 @@ public void testSingleClusteringInvalidQueries() throws
Throwable
assertInvalidSyntax("SELECT * FROM %s WHERE () = (?, ?)", 1, 2);
assertInvalidMessage("b cannot be restricted by more than one relation
if it includes an Equal",
"SELECT * FROM %s WHERE a = 0 AND (b) = (?) AND
(b) > (?)", 0, 0);
+ assertInvalidMessage("b cannot be restricted by more than one relation
if it includes an Equal",
Review Comment:
I would probably add something to `testMultiClusteringInvalidQueries`, like:
```
assertInvalidMessage("Invalid null value for column b",
"SELECT * FROM %s WHERE a = 0 AND b BETWEEN ? AND ?", 1,
null);
```
##########
src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java:
##########
@@ -470,4 +424,80 @@ public ClusteringBound<?> toBound(boolean isStart, boolean
isInclusive)
return isEmpty() ? BufferClusteringBound.TOP :
super.toBound(isStart, isInclusive);
}
}
+
+ public static class ClusteringElementsComparator implements
Comparator<ClusteringElements>
Review Comment:
I would add some Java doc for this comparator and mostly for the `forCQL`.
It should be clear without checking the usages.
##########
test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java:
##########
@@ -63,6 +63,8 @@ public void testInvalidCollectionNonEQRelation() throws
Throwable
"SELECT * FROM %s WHERE c = 0 AND b <= ?",
set(0));
assertInvalidMessage("Collection column 'b' (set<int>) cannot be
restricted by a 'IN' relation",
"SELECT * FROM %s WHERE c = 0 AND b IN (?)",
set(0));
+ assertInvalidMessage("Collection column 'b' (set<int>) cannot be
restricted by a 'BETWEEN' relation",
Review Comment:
Shall we add some test case to
`testClusteringColumnRelationsWithClusteringOrder` and `testWithUnsetValues` in
this class?
--
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]
