CASSANDRA-19604 Add support for BETWEEN operator #3301

xvade · 2024-05-10T20:00:30Z

https://issues.apache.org/jira/browse/CASSANDRA-19604

blerer

Nice work! I left a few comments for the parts that could be improved and the ones I forgot to mention before.

src/java/org/apache/cassandra/cql3/Operator.java

src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java

blerer · 2024-05-13T13:40:02Z

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)


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.

I am wondering why BETWEEN(16) is not after 15?

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.

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.

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.

src/java/org/apache/cassandra/cql3/Operator.java

ekaterinadimitrova2

Overall looks great, I left some very tiny comments.
Question: I see you updated the ticket about NOT BETWEEN, do you plan to add it later?

src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java

test/unit/org/apache/cassandra/cql3/restrictions/ClusteringElementsTest.java

test/unit/org/apache/cassandra/cql3/validation/operations/DeleteTest.java

test/unit/org/apache/cassandra/cql3/validation/operations/SelectMultiColumnRelationTest.java

src/java/org/apache/cassandra/cql3/restrictions/ClusteringElements.java

test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java

ekaterinadimitrova2 · 2024-05-20T23:28:20Z

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', '<', '>'])


We do not support BETWEEN in UPDATE?

We discuss that with @ekaterinadimitrova2 . The issue is that this part is common to all the statements using the WHERE clause.

blerer

I have added 2 changes proposals here

blerer · 2024-05-21T07:33:51Z

src/antlr/Parser.g

+      | K_BETWEEN literals=betweenLiterals
+                  { $clauses.add(Relation.multiColumn(ids, Operator.BETWEEN, literals)); }


The current change support literals (BETWEEN 1 AND 2) but not markers (BETWEEN ? AND ?).

src/antlr/Parser.g

blerer · 2024-05-21T11:53:40Z

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;


you can combine them in a single assertion assert operator != Operator.IN && operator != Operator.BETWEEN;

blerer · 2024-05-21T12:23:04Z

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)


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.

…ting, and documentation.

…dBy(). Added ClusteringElementsComparator to ClusteringElements.java, reworked ClusteringElements.compareTo() to make use of it. Added test for BETWEEN to RelationTest.java and updated Relation to pass it.

…ce operators. Added BETWEEN to the reserved_keywords.txt.

…nNames().

blerer reviewed May 13, 2024

View reviewed changes

ekaterinadimitrova2 reviewed May 14, 2024

View reviewed changes

src/java/org/apache/cassandra/cql3/Operator.java Outdated Show resolved Hide resolved

xvade force-pushed the between branch 3 times, most recently from eb4203f to 7ae6453 Compare May 20, 2024 19:09

ekaterinadimitrova2 reviewed May 20, 2024

View reviewed changes

blerer reviewed May 21, 2024

View reviewed changes

xvade force-pushed the between branch 2 times, most recently from 1c51f92 to af04297 Compare May 28, 2024 23:32

xvade added 9 commits June 2, 2024 12:38

Added BETWEEN to Operator.java with appropriate support in antlr, tes…

98ae976

…ting, and documentation.

Fixed some errors related to the rebase.

39c067f

Streamlined Operator.java's BETWEEN.restrict() and BETWEEN.isSatisfie…

5c6cc11

…dBy(). Added ClusteringElementsComparator to ClusteringElements.java, reworked ClusteringElements.compareTo() to make use of it. Added test for BETWEEN to RelationTest.java and updated Relation to pass it.

Reverted a bad formatting change to Operator.java.

37be05c

Added testForCQLComparator to ClusteringElementsTest.java.

391d756

Made adjustments according to comments on pull request.

08eb8c1

Cleaned up code in accordance to suggestions from blerer.

55c69bc

Updated BETWEEN's requiresFilteringOrIndexingFor() to match other sli…

dcd0f00

…ce operators. Added BETWEEN to the reserved_keywords.txt.

Complying with suggestions from blerer at apache@428ce78.

45c6c24

xvade force-pushed the between branch from 2219758 to 45c6c24 Compare June 2, 2024 19:38

Fixed an issue with SelectSingleColumnRelationTest's testInvalidColum…

5601f89

…nNames().

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-19604 Add support for BETWEEN operator #3301

CASSANDRA-19604 Add support for BETWEEN operator #3301

xvade commented May 10, 2024

blerer left a comment

blerer May 13, 2024

ekaterinadimitrova2 May 14, 2024

xvade May 14, 2024 •

edited

blerer May 21, 2024

ekaterinadimitrova2 May 21, 2024 •

edited

ekaterinadimitrova2 left a comment

ekaterinadimitrova2 May 20, 2024

xvade May 22, 2024

blerer May 30, 2024

blerer left a comment

blerer May 21, 2024

blerer May 21, 2024

blerer May 21, 2024

		\| K_BETWEEN literals=betweenLiterals
		{ $clauses.add(Relation.multiColumn(ids, Operator.BETWEEN, literals)); }

CASSANDRA-19604 Add support for BETWEEN operator #3301

Are you sure you want to change the base?

CASSANDRA-19604 Add support for BETWEEN operator #3301

Conversation

xvade commented May 10, 2024

blerer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xvade May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekaterinadimitrova2 May 21, 2024 • edited

Choose a reason for hiding this comment

ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blerer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xvade May 14, 2024 •

edited

ekaterinadimitrova2 May 21, 2024 •

edited