Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 1.2.8, 2.0 rc1
    • Component/s: None
    • Labels:

      Description

      It would be nice to have support of empty IN queries.
      Example: "SELECT a FROM t WHERE aKey IN ()".
      One of the reasons is to have such support in DataStax Java Driver (see discussion here: https://datastax-oss.atlassian.net/browse/JAVA-106).

      1. 5626-v2-UPDATE-DELETE.txt
        2 kB
        Aleksey Yeschenko
      2. 5626-v2.txt
        12 kB
        Aleksey Yeschenko
      3. 5626.txt
        13 kB
        Aleksey Yeschenko

        Activity

        Alexander Solovyev created issue -
        Alexander Solovyev made changes -
        Field Original Value New Value
        Description It would be nice to have support of empty IN queries. E.g. "SELECT a FROM t WHERE aKey IN ()".
        One of the reasons is to have such support in DataStax Java Driver (see discussion here: https://datastax-oss.atlassian.net/browse/JAVA-106).
        It would be nice to have support of empty IN queries, an example: "SELECT a FROM t WHERE aKey IN ()".
        One of the reasons is to have such support in DataStax Java Driver (see discussion here: https://datastax-oss.atlassian.net/browse/JAVA-106).
        Alexander Solovyev made changes -
        Issue Type Wish [ 5 ] Improvement [ 4 ]
        Alexander Solovyev made changes -
        Description It would be nice to have support of empty IN queries, an example: "SELECT a FROM t WHERE aKey IN ()".
        One of the reasons is to have such support in DataStax Java Driver (see discussion here: https://datastax-oss.atlassian.net/browse/JAVA-106).
        It would be nice to have support of empty IN queries.
        Example: "SELECT a FROM t WHERE aKey IN ()".
        One of the reasons is to have such support in DataStax Java Driver (see discussion here: https://datastax-oss.atlassian.net/browse/JAVA-106).
        Hide
        Jonathan Ellis added a comment -

        WDYT Aleksey?

        Show
        Jonathan Ellis added a comment - WDYT Aleksey?
        Jonathan Ellis made changes -
        Assignee Aleksey Yeschenko [ iamaleksey ]
        Priority Major [ 3 ] Minor [ 4 ]
        Hide
        Aleksey Yeschenko added a comment -

        WDYT Aleksey?

        I think it makes sense. Should be a trivial patch, too.

        Show
        Aleksey Yeschenko added a comment - WDYT Aleksey? I think it makes sense. Should be a trivial patch, too.
        Hide
        Alexander Solovyev added a comment -

        Thank you, guys. Looking forward.

        Show
        Alexander Solovyev added a comment - Thank you, guys. Looking forward.
        Aleksey Yeschenko made changes -
        Attachment 5626.txt [ 12593860 ]
        Aleksey Yeschenko made changes -
        Labels cql3
        Affects Version/s 1.2.0 [ 12323243 ]
        Reviewer slebresne
        Aleksey Yeschenko made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Sylvain Lebresne added a comment -

        On the patch:

        • The "simplification" in getKeyBounds is not completely correct. If we do a 2ndary index query with a EQ on the partition key (which is supported), we will still go through getKeyBounds even though we don't use the token function (more precisely, this patch breaks the cql_tests.py:TestCQL.indexed_with_eq_test dtest) We can still simplify things a bit, because we don't need the current generality, but I think I'd rather open a separate ticket for that.
        • SelectStatement.getSliceCommands() was assuming that names filters were never null, which is not the case anymore with this patch. Concretely, I've pushed a test for this in dtests (http://goo.gl/VpqD46) and with the attached patch there's a NPE thrown by this test.
        • Why remove the break when checking if we have at least one indexed EQ clause?
        • Nit: in getIndexExpressions, let's add an assert that we're not in the IN case, if only for documenting that we only use the first element on purpose.
        Show
        Sylvain Lebresne added a comment - On the patch: The "simplification" in getKeyBounds is not completely correct. If we do a 2ndary index query with a EQ on the partition key (which is supported), we will still go through getKeyBounds even though we don't use the token function (more precisely, this patch breaks the cql_tests.py:TestCQL.indexed_with_eq_test dtest) We can still simplify things a bit, because we don't need the current generality, but I think I'd rather open a separate ticket for that. SelectStatement.getSliceCommands() was assuming that names filters were never null, which is not the case anymore with this patch. Concretely, I've pushed a test for this in dtests ( http://goo.gl/VpqD46 ) and with the attached patch there's a NPE thrown by this test. Why remove the break when checking if we have at least one indexed EQ clause? Nit: in getIndexExpressions, let's add an assert that we're not in the IN case, if only for documenting that we only use the first element on purpose.
        Hide
        Aleksey Yeschenko added a comment -

        Why remove the break when checking if we have at least one indexed EQ clause?

        Otherwise https://github.com/apache/cassandra/blob/cassandra-1.2/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java#L1160 validation is non-deterministically reached - depending on the names of the indexed columns and on the hashmap seed (and then getIndexExpressions get(0) throws NPE with IN () half the time). So you need to iterate over all restrictions.

        Show
        Aleksey Yeschenko added a comment - Why remove the break when checking if we have at least one indexed EQ clause? Otherwise https://github.com/apache/cassandra/blob/cassandra-1.2/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java#L1160 validation is non-deterministically reached - depending on the names of the indexed columns and on the hashmap seed (and then getIndexExpressions get(0) throws NPE with IN () half the time). So you need to iterate over all restrictions.
        Aleksey Yeschenko made changes -
        Attachment 5626-v2.txt [ 12593943 ]
        Hide
        Aleksey Yeschenko added a comment -

        We can still simplify things a bit, because we don't need the current generality, but I think I'd rather open a separate ticket for that.

        Agreed. Removed it.

        SelectStatement.getSliceCommands() was assuming that names filters were never null, which is not the case anymore with this patch.

        Oops, missed 1/3. Corrected.

        Nit: in getIndexExpressions, let's add an assert that we're not in the IN case, if only for documenting that we only use the first element on purpose.

        Addressed.

        Show
        Aleksey Yeschenko added a comment - We can still simplify things a bit, because we don't need the current generality, but I think I'd rather open a separate ticket for that. Agreed. Removed it. SelectStatement.getSliceCommands() was assuming that names filters were never null, which is not the case anymore with this patch. Oops, missed 1/3. Corrected. Nit: in getIndexExpressions, let's add an assert that we're not in the IN case, if only for documenting that we only use the first element on purpose. Addressed.
        Aleksey Yeschenko made changes -
        Fix Version/s 1.2.8 [ 12324785 ]
        Hide
        Sylvain Lebresne added a comment -

        v2 lgtm, +1

        Show
        Sylvain Lebresne added a comment - v2 lgtm, +1
        Aleksey Yeschenko made changes -
        Attachment 5626-v2-UPDATE-DELETE.txt [ 12594284 ]
        Hide
        Aleksey Yeschenko added a comment -

        https://datastax-oss.atlassian.net/browse/JAVA-106 mentioned adding IN () support for DELETE (and forgot to mention UPDATE). v2 didn't add that, so attaching 5626-v2-UPDATE-DELETE.txt to handle that.

        Show
        Aleksey Yeschenko added a comment - https://datastax-oss.atlassian.net/browse/JAVA-106 mentioned adding IN () support for DELETE (and forgot to mention UPDATE). v2 didn't add that, so attaching 5626-v2-UPDATE-DELETE.txt to handle that.
        Hide
        Alexander Solovyev added a comment -

        and forgot to mention UPDATE

        Well, the https://datastax-oss.atlassian.net/browse/JAVA-106 is about overall IN () support

        Show
        Alexander Solovyev added a comment - and forgot to mention UPDATE Well, the https://datastax-oss.atlassian.net/browse/JAVA-106 is about overall IN () support
        Hide
        Sylvain Lebresne added a comment -

        +1 on that last patch

        Show
        Sylvain Lebresne added a comment - +1 on that last patch
        Hide
        Aleksey Yeschenko added a comment -

        Committed, thanks.

        Show
        Aleksey Yeschenko added a comment - Committed, thanks.
        Aleksey Yeschenko made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Jonathan Ellis made changes -
        Fix Version/s 2.0 rc1 [ 12324787 ]
        Aleksey Yeschenko made changes -
        Component/s Core [ 12312978 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        44d 20h 15m 1 Aleksey Yeschenko 24/Jul/13 06:21
        Patch Available Patch Available Resolved Resolved
        2d 11h 25m 1 Aleksey Yeschenko 26/Jul/13 17:46

          People

          • Assignee:
            Aleksey Yeschenko
            Reporter:
            Alexander Solovyev
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development