Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 1.2.8, 2.0 rc1
    • Component/s: Core
    • 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

        Hide
        Aleksey Yeschenko added a comment -

        Committed, thanks.

        Show
        Aleksey Yeschenko added a comment - Committed, thanks.
        Hide
        Sylvain Lebresne added a comment -

        +1 on that last patch

        Show
        Sylvain Lebresne added a comment - +1 on that last patch
        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
        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
        Sylvain Lebresne added a comment -

        v2 lgtm, +1

        Show
        Sylvain Lebresne added a comment - v2 lgtm, +1
        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.
        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.
        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
        Alexander Solovyev added a comment -

        Thank you, guys. Looking forward.

        Show
        Alexander Solovyev added a comment - Thank you, guys. Looking forward.
        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
        Jonathan Ellis added a comment -

        WDYT Aleksey?

        Show
        Jonathan Ellis added a comment - WDYT Aleksey?

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development