You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "blerer (via GitHub)" <gi...@apache.org> on 2024/06/03 15:47:22 UTC
[PR] CASSANDRA-19604: BETWEEN [cassandra]
blerer opened a new pull request, #3345:
URL: https://github.com/apache/cassandra/pull/3345
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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: BETWEEN [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on PR #3345:
URL: https://github.com/apache/cassandra/pull/3345#issuecomment-2146011346
On a quick look, the last few commits make sense, just missing Caleb's suggestion from Slack - https://the-asf.slack.com/archives/C018YGVCHMZ/p1717430564268499
I looked around the CI run, and indeed, the Java test failures are the only suspicious ones; I did not find any tickets.
They may be flaky, `MultiNodeSAITest` is a mighty fuzz test. I hope the seed with which it failed in CI is available in the logs. Also, we need `for i in {1..9}; do sudo ifconfig lo0 alias 127.0.0.$i up; done;` to be able to run it locally. It is similar to the Python tests but undocumented, and I keep forgetting that detail.
I will make a full pass of the whole work when the latest changes land.
--
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: BETWEEN [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #3345:
URL: https://github.com/apache/cassandra/pull/3345#discussion_r1627970590
##########
src/java/org/apache/cassandra/cql3/Relation.java:
##########
@@ -225,6 +225,11 @@ public int hashCode()
*/
public String toCQLString()
{
+ if (operator.isTernary())
+ {
+ List<? extends Term.Raw> terms = rawTerms.asList();
+ return String.format("%s %s %s AND %s", rawExpressions.toCQLString(), operator, terms.get(0), terms.get(1));
Review Comment:
Checked that it is covered by a unit test, thanks
##########
src/java/org/apache/cassandra/cql3/Relation.java:
##########
@@ -72,7 +72,7 @@ public Operator operator()
*/
public static Relation singleColumn(ColumnIdentifier identifier, Operator operator, Term.Raw rawTerm)
{
- assert operator != Operator.IN;
+ assert operator.kind() == Operator.Kind.BINARY;
Review Comment:
So this version of `singleColumn` is used for everything but IN and BETWEEN.
Please add assertions for the other version `assert operator.kind() != Operator.Kind.BINARY;`
Same for `multiColumn`. I guess it can be done on commit
##########
src/java/org/apache/cassandra/db/filter/RowFilter.java:
##########
@@ -748,6 +748,15 @@ protected String toString(boolean cql)
default:
break;
}
+
+ if (operator.isTernary())
+ {
+ List<? extends ByteBuffer> buffers = ((ListType<?>) type).unpack(value);
+ return cql
+ ? String.format("%s %s %s AND %s", column.name.toCQLString(), operator, type.toCQLString(buffers.get(0)), type.toCQLString(buffers.get(1)))
Review Comment:
We do have some test for this one? Just double-checking
##########
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:
In regards to https://github.com/apache/cassandra/pull/3301/files#r1607246867 and testDeleteWithNonoverlappingRange, below test pass, we can add it on commit?
```
@Test
public void testDeleteWithNonoverlappingRangeUsingBetween() throws Throwable
{
createTable("CREATE TABLE %s (a int, b int, c text, primary key (a, b))");
for (int i = 0; i < 10; i++)
execute("INSERT INTO %s (a, b, c) VALUES(1, ?, 'abc')", i);
flush();
execute("DELETE FROM %s WHERE a=1 and b BETWEEN 0 AND 3");
flush();
// this query does not overlap the tombstone range above and caused the rows to be resurrected
assertEmpty(execute("SELECT * FROM %s WHERE a=1 and b BETWEEN 0 AND 2"));
}
```
--
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: BETWEEN [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #3345:
URL: https://github.com/apache/cassandra/pull/3345#issuecomment-2146149718
@ekaterinadimitrova2 If `MultiNodeSAITest` isn't something trivial, or the problem reproduces w/ that seed on trunk, definitely let me know...
--
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