You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "xvade (via GitHub)" <gi...@apache.org> on 2024/05/10 20:00:33 UTC
[PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
xvade opened a new pull request, #3301:
URL: https://github.com/apache/cassandra/pull/3301
https://issues.apache.org/jira/browse/CASSANDRA-19604
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1608525619
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I was wondering, maybe we want to add a defensive unit test that will check the numbers used and break if someone does something wrong?
```
@Test
public void testOperatorOrder()
{
int[] expectedValues = {0, 4, 3, 1, 2, 7, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15};
Operator[] operators = Operator.values();
assertEquals("Number of operators has changed", expectedValues.length, operators.length);
for (int i = 0; i < operators.length; i++)
{
assertEquals("Operator " + operators[i].name() + " has incorrect value", expectedValues[i], operators[i].getValue());
}
}
```
This test will fail if the number of operators or the b value of any operator is changed. It will also fail if a new operator is added but does not take the next number in line.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1599239546
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -1,4 +1,4 @@
-/*
+ /*
Review Comment:
I suspect this was unintentional formatting change. Please revert it
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "blerer (via GitHub)" <gi...@apache.org>.
blerer commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1598472034
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
+ {
+ @Override
+ public String toString()
+ {
+ return "BETWEEN";
+ }
+
+ @Override
+ public boolean isSatisfiedBy(AbstractType<?> type, ByteBuffer leftOperand, ByteBuffer rightOperand)
+ {
+ ListSerializer<?> serializer = ListType.getInstance(type, false).getSerializer();
+
+ ByteBuffer leftBound = serializer.getElement(rightOperand, 0);
+ ByteBuffer rightBound = serializer.getElement(rightOperand, 1);
+
+ ByteBuffer larger = type.compareForCQL(leftBound, rightBound) > 0 ? leftBound : rightBound;
+ ByteBuffer smaller = type.compareForCQL(larger, leftOperand) == 0 ? rightBound : leftBound;
+
+ return type.compareForCQL(leftOperand, larger) <= 0 && type.compareForCQL(leftOperand, smaller) >= 0;
+ }
+
+ @Override
+ public boolean requiresFilteringOrIndexingFor(ColumnMetadata.Kind columnKind)
+ {
+ return !columnKind.isPrimaryKeyKind();
+ }
+
+ @Override
+ public void restrict(RangeSet<ClusteringElements> rangeSet, List<ClusteringElements> args)
+ {
+ assert args.size() == 2 : this + " accepts exactly two values";
+ ClusteringElements left = args.get(0);
+ ClusteringElements right = args.get(1);
+ int comp = ClusteringElements.compareForCQL(left, right);
Review Comment:
I would use a `Comparator` instead of a method as it would allow you to simply sort the `List`.
##########
src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java:
##########
@@ -370,6 +370,65 @@ public int compareTo(ClusteringElements that)
: that instanceof Top ? -1 : 1;
}
+ // Very similar to compareTo, the differences are: compareForCQL is static, compareForCQL unwraps column types
+ // before comparing with them.
+ public static int compareForCQL(ClusteringElements a, ClusteringElements b)
Review Comment:
CQL values cannot be represented as bounds (`Top` or `Bottom`) so you should reject those. That should allow you to simplify the logic greatly.
Do not forget to add tests in` ClusteringElementsTest`
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I did not thought about it before but I am guessing that `BETWEEN` is breaking the `toString` method of `Relation` and `SimpleRestriction`. `RelationTest` contains some tests for the `Relation` method.
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
+ {
+ @Override
+ public String toString()
+ {
+ return "BETWEEN";
+ }
+
+ @Override
+ public boolean isSatisfiedBy(AbstractType<?> type, ByteBuffer leftOperand, ByteBuffer rightOperand)
+ {
+ ListSerializer<?> serializer = ListType.getInstance(type, false).getSerializer();
+
+ ByteBuffer leftBound = serializer.getElement(rightOperand, 0);
+ ByteBuffer rightBound = serializer.getElement(rightOperand, 1);
+
+ ByteBuffer larger = type.compareForCQL(leftBound, rightBound) > 0 ? leftBound : rightBound;
+ ByteBuffer smaller = type.compareForCQL(larger, leftOperand) == 0 ? rightBound : leftBound;
+
+ return type.compareForCQL(leftOperand, larger) <= 0 && type.compareForCQL(leftOperand, smaller) >= 0;
Review Comment:
The logic could be simplified by using `ListType.unpack`:
```
List<ByteBuffer> buffers = ListType.getInstance(type, false).unpack(rightOperand);
Collections.sort(buffers, type);
return type.compareForCQL(leftOperand, buffers.get(1)) <= 0 && type.compareForCQL(leftOperand, buffers.get(0)) >= 0;
```
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin closed pull request #3301: CASSANDRA-19604 Add support for BETWEEN operator
URL: https://github.com/apache/cassandra/pull/3301
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "blerer (via GitHub)" <gi...@apache.org>.
blerer commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1607788871
##########
src/antlr/Parser.g:
##########
@@ -1797,6 +1811,8 @@ relation[WhereClause.Builder clauses]
}
| type=relationType tupleMarker=markerForTuple /* (a, b, c) >= ? */
{ $clauses.add(Relation.multiColumn(ids, type, tupleMarker)); }
+ | K_BETWEEN literals=betweenLiterals
+ { $clauses.add(Relation.multiColumn(ids, Operator.BETWEEN, literals)); }
Review Comment:
The current change support literals (`BETWEEN 1 AND 2`) but not markers (`BETWEEN ? AND ?`).
##########
src/java/org/apache/cassandra/cql3/Relation.java:
##########
@@ -114,6 +115,7 @@ public static Relation mapElement(ColumnIdentifier identifier, Term.Raw rawKey,
public static Relation multiColumn(List<ColumnIdentifier> identifiers, Operator operator, Term.Raw rawTerm)
{
assert operator != Operator.IN;
+ assert operator != Operator.BETWEEN;
Review Comment:
you can combine them in a single assertion `assert operator != Operator.IN && operator != Operator.BETWEEN;`
##########
src/antlr/Parser.g:
##########
@@ -1768,6 +1780,8 @@ relationType returns [Operator op]
relation[WhereClause.Builder clauses]
: name=cident type=relationType t=term { $clauses.add(Relation.singleColumn(name, type, t)); }
+ | name=cident K_BETWEEN betweenValues=singleColumnBetweenValues
Review Comment:
an alternative could be
`| name=cident K_BETWEEN t1=term K_AND t2=term { $clauses.add(Relation.singleColumn($name.id, Operator.BETWEEN, Arrays.asList(t1, t2)); } `
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
What you say make sense. There is a separation between the serialization value of an Operator and the enum order. Now, the issue is that people will pick the last number of the list and use the next one for the serialization of the next enum they will add. It is not perfect but I will still put between at the end to avoid this issue that can make people lose time trying to figure out what is going on when they run into an issue with the tests.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "xvade (via GitHub)" <gi...@apache.org>.
xvade commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1610574697
##########
src/antlr/Parser.g:
##########
@@ -1768,6 +1780,8 @@ relationType returns [Operator op]
relation[WhereClause.Builder clauses]
: name=cident type=relationType t=term { $clauses.add(Relation.singleColumn(name, type, t)); }
+ | name=cident K_BETWEEN betweenValues=singleColumnBetweenValues
Review Comment:
I've tried getting rid of `singleColumnBetweenValues` a few different ways, including what you suggested verbatim and nothing has worked. It keeps giving me `error: incompatible types: Raw cannot be converted to Literal`. I'm keeping the working code for now, if you say this is important, I'll keep looking for a way.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "blerer (via GitHub)" <gi...@apache.org>.
blerer commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1620993197
##########
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 discuss that with @ekaterinadimitrova2 . The issue is that this part is common to all the statements using the WHERE clause.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "xvade (via GitHub)" <gi...@apache.org>.
xvade commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1609085702
##########
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:
Oops!
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1608525619
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I was wondering, maybe we want to add a defensive unit test that will check the numbers used and break if someone does something wrong?
```
@Test
public void testOperatorBinaryRepresentation()
{
int[] expectedValues = {0, 4, 3, 1, 2, 7, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15};
Operator[] operators = Operator.values();
assertEquals("Number of operators has changed", expectedValues.length, operators.length);
for (int i = 0; i < operators.length; i++)
{
assertEquals("Operator " + operators[i].name() + " has incorrect value", expectedValues[i], operators[i].getValue());
}
}
```
This test will fail if the number of operators or the b value of any operator is changed. It will also fail if a new operator is added but does not take the next number in line. We can add a comment for people to update it when it fails on new operator addition.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "xvade (via GitHub)" <gi...@apache.org>.
xvade commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1599243523
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
> I am wondering why `BETWEEN(16)` is not after 15?
It seemed that the convention in Operator.java was not super strict about ordering operations numerically, and so I opted to put it with the slices because that seemed like a fitting way of ordering in lieu of by index. It would also make a ton of sense to put it after 15, but I'd be inclined to reorder or renumber the rest of the file to fit the numbering convention if we did that.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1608525619
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I was wondering, maybe we want to add a defensive unit test that will check the numbers used and break if someone does something wrong?
```
@Test
public void testOperatorOrder()
{
int[] expectedValues = {0, 4, 3, 1, 2, 7, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15};
Operator[] operators = Operator.values();
assertEquals("Number of operators has changed", expectedValues.length, operators.length);
for (int i = 0; i < operators.length; i++)
{
assertEquals("Operator " + operators[i].name() + " has incorrect value", expectedValues[i], operators[i].getValue());
}
}
```
This test will fail if the number of operators or the b value of any operator is changed. It will also fail if a new operator is added but does not take the next number in line. We can add a comment for people to update it when it fails on new operator addition.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1608525619
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I was wondering, maybe we want to add a defensive unit test that will check the numbers used and break if someone does something wrong?
```
@Test
public void testOperatorOrder() {
int[] expectedValues = {0, 4, 3, 1, 2, 7, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15};
Operator[] operators = Operator.values();
assertEquals("Number of operators has changed", expectedValues.length, operators.length);
for (int i = 0; i < operators.length; i++) {
assertEquals("Operator " + operators[i].name() + " has incorrect value", expectedValues[i], operators[i].getValue());
}
}
```
This test will fail if the number of operators or the b value of any operator is changed. It will also fail if a new operator is added but does not take the next number in line.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin commented on PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#issuecomment-4227138503
Closing as [CASSANDRA-19604](https://issues.apache.org/jira/browse/CASSANDRA-19604) has been resolved as Fixed — if this PR contains changes not captured in the merged fix, please reopen.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#issuecomment-2555002426
This is merged, can it be closed?
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1599240070
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I am wondering why `BETWEEN(16)` is not after 15?
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "xvade (via GitHub)" <gi...@apache.org>.
xvade commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1599243523
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
> I am wondering why `BETWEEN(16)` is not after 15?
It seemed that the convention in Operator.java was not super strict about ordering operations numerically, and so I opted to put it with the slices because that seemed like a fitting way of ordering in lieu of by index.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3301:
URL: https://github.com/apache/cassandra/pull/3301#discussion_r1608525619
##########
src/java/org/apache/cassandra/cql3/Operator.java:
##########
@@ -266,6 +266,59 @@ public boolean canBeUsedWith(ColumnsExpression.Kind kind)
return kind != ColumnsExpression.Kind.MAP_ELEMENT;
}
},
+ BETWEEN(16)
Review Comment:
I was wondering, maybe we want to add a defensive unit test that will check the numbers used and break if someone does something wrong or when they add a new operator (they will have to update it)?
```
@Test
public void testOperatorBinaryRepresentation()
{
int[] expectedValues = {0, 4, 3, 1, 2, 7, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15};
Operator[] operators = Operator.values();
assertEquals("Number of operators has changed", expectedValues.length, operators.length);
for (int i = 0; i < operators.length; i++)
{
assertEquals("Operator " + operators[i].name() + " has incorrect value", expectedValues[i], operators[i].getValue());
}
}
```
This test will fail if the number of operators or the b value of any operator is changed. It will also fail if a new operator is added but does not take the next number in line. We can add a comment for people to update it when it fails on new operator addition.
--
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
Re: [PR] CASSANDRA-19604 Add support for BETWEEN operator [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
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: 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