Cassandra
  1. Cassandra
  2. CASSANDRA-2737

CQL: support IF EXISTS extension for DROP commands (table, keyspace, index)

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Fix Version/s: 2.0 beta 1
    • Component/s: None
    • Labels:
    1. 2737-v2.txt
      19 kB
      Michał Michalski
    2. 2737-poor-mans-testcase.cql
      0.8 kB
      Michał Michalski
    3. 2737-concept-v1.txt
      13 kB
      Michał Michalski

      Activity

      Sylvain Lebresne made changes -
      Status Patch Available [ 10002 ] Resolved [ 5 ]
      Fix Version/s 2.0 beta 1 [ 12322954 ]
      Fix Version/s 2.1 [ 12324159 ]
      Resolution Fixed [ 1 ]
      Hide
      Sylvain Lebresne added a comment -

      Alright, lgtm, I've committed this to trunk.

      I did made a few minor simplification upon commit, including fixing the code style (that wasn't matching the one we use) and updating the documentation.

      Show
      Sylvain Lebresne added a comment - Alright, lgtm, I've committed this to trunk. I did made a few minor simplification upon commit, including fixing the code style (that wasn't matching the one we use) and updating the documentation.
      Michał Michalski made changes -
      Attachment 2737-v2.txt [ 12588857 ]
      Hide
      Michał Michalski added a comment -

      Replacing old v2 with v2 rebased onto current trunk.

      Show
      Michał Michalski added a comment - Replacing old v2 with v2 rebased onto current trunk.
      Michał Michalski made changes -
      Attachment 2737-v2.txt [ 12585169 ]
      Jonathan Ellis made changes -
      Reviewer slebresne
      Michał Michalski made changes -
      Status Open [ 1 ] Patch Available [ 10002 ]
      Labels cql cql cql3
      Assignee Michał Michalski [ michalm ]
      Michał Michalski made changes -
      Attachment 2737-v2.txt [ 12585169 ]
      Attachment 2737-poor-mans-testcase.cql [ 12585170 ]
      Hide
      Michał Michalski added a comment -

      Attaching patch + "poor man's testcase" (set of CQL commands)

      Show
      Michał Michalski added a comment - Attaching patch + "poor man's testcase" (set of CQL commands)
      Jonathan Ellis made changes -
      Fix Version/s 2.1 [ 12324159 ]
      Fix Version/s 2.0 [ 12322954 ]
      Hide
      Michał Michalski added a comment -

      Thanks for reply, Sylvain!

      I definitely agree that my solution has some overhead and your is much simpler, but one of the things I aimed for was to distinguish the "critical" exceptions from the ones that can be handled by such "IF (NOT) EXISTS" extensions, so - in the end - it could be possible to let user know what really happened and, maybe, display some descriptive NOTICE message instead of displaying nothing (I was a bit "inspired" in how it's reported in PostgreSQL in similar case). However, if you say it's not necessary, I'm fine with it

      Additionally, in most of the cases validation checking if KS/CF/Index exists (or not) happens outside *Statement classes (for example for CFs it's done in MigrationManager.announceColumnFamilyDrop()) and I didn't like the idea of passing additional parameter and handling it there - I preferred this decision (if exception should be thrown or not) to be done by *Statement class itself, basing on what was returned/thrown by MigrationManager.

      One thing I missed when coding was the fact that for KS/CF, when reporting existing / inexistent KS/CF, ConfigurationException is thrown (index-related code throws InvalidRequestException, which looks like a little inconsistency to me, by the way), so I could have reused it, instead of creating new ones.

      Anyway, I see your point Tomorrow I'm leaving for three weeks and when I'm back I'll reimplement it taking your suggestions into account

      Show
      Michał Michalski added a comment - Thanks for reply, Sylvain! I definitely agree that my solution has some overhead and your is much simpler, but one of the things I aimed for was to distinguish the "critical" exceptions from the ones that can be handled by such "IF (NOT) EXISTS" extensions, so - in the end - it could be possible to let user know what really happened and, maybe, display some descriptive NOTICE message instead of displaying nothing (I was a bit "inspired" in how it's reported in PostgreSQL in similar case). However, if you say it's not necessary, I'm fine with it Additionally, in most of the cases validation checking if KS/CF/Index exists (or not) happens outside *Statement classes (for example for CFs it's done in MigrationManager.announceColumnFamilyDrop()) and I didn't like the idea of passing additional parameter and handling it there - I preferred this decision (if exception should be thrown or not) to be done by *Statement class itself, basing on what was returned/thrown by MigrationManager. One thing I missed when coding was the fact that for KS/CF, when reporting existing / inexistent KS/CF, ConfigurationException is thrown (index-related code throws InvalidRequestException, which looks like a little inconsistency to me, by the way), so I could have reused it, instead of creating new ones. Anyway, I see your point Tomorrow I'm leaving for three weeks and when I'm back I'll reimplement it taking your suggestions into account
      Hide
      Sylvain Lebresne added a comment -

      Seems a bit more complicated that what I would I gone for a priori. It seems to me that what we'd need for CREATE is:

      1. add a ifNotExist boolean to the existing statement.
      2. in validate, if the CF already exists, just skip throwing an exception. And in execute, if the CF exists, skip the creation.

      And the equivalent for DROP.

      Doing this wouldn't allow the user to know if the creation did happen or if it was already existing, but I think it's fair since you can always use the current statement (and catch the exception) if you care about that. I suppose we could alternatively return a result set with a boolean indicating if creation did happen, for convenience sake, but I'm not even sure that'd be so useful.

      But refactoring QueryProcessor and adding 2 new exceptions/new statements sounds overkill to me.

      Show
      Sylvain Lebresne added a comment - Seems a bit more complicated that what I would I gone for a priori. It seems to me that what we'd need for CREATE is: add a ifNotExist boolean to the existing statement. in validate, if the CF already exists, just skip throwing an exception. And in execute, if the CF exists, skip the creation. And the equivalent for DROP. Doing this wouldn't allow the user to know if the creation did happen or if it was already existing, but I think it's fair since you can always use the current statement (and catch the exception) if you care about that. I suppose we could alternatively return a result set with a boolean indicating if creation did happen, for convenience sake, but I'm not even sure that'd be so useful. But refactoring QueryProcessor and adding 2 new exceptions/new statements sounds overkill to me.
      Michał Michalski made changes -
      Attachment 2737-concept-v1.txt [ 12578596 ]
      Hide
      Michał Michalski added a comment -

      Here's my idea of implementing this feature (see attached file for details):

      1. Move the validate(), checkAccess() and execute() method calls from QueryProcessor into separate "wrapping" method (I called it validateCheckAccessAndExecute for now ) defined in CQLStatement interface. All the classes implementing this interface will just execute these three methods one by one, like QueryProcessor did (a bit non-DRY). By doing so we can handle the exceptions thrown by these methods "internally" (in specific Statement classes), without adding any logic to QueryProcessor.
      2. Define two exceptions inheriting from InvalidRequestException so we can distinguish when we can (if IF [NOT] EXISTS was used in the query) just ignore a problem that occured - let's call them: MissingObject and ObjectExists. They are - obviously - thrown when we try to create existing object or drop the one that does not exist.
      3. Define two subclasses of SchemaAlteringStatement: DropStatement and CreateStatement - they require one additional parameter in constructor (indicating if IF [NOT] EXISTS statement was used) and override the validateCheckAccessAndExecute() method to check if any of the exceptions mentioned above was thrown and - if allowed - handle it by returning null (converted to Void message in QueryProcessor) or re-throw the exception.

      I've implemented this solution for DROP INDEX statement as an example - see attachment. If you're OK with this implementation, I'll do the rest.

      For now there are no changes in cqlsh (so autocompletion does not work), but of course it's also in the scope of this task.

      Important QueryProcessor code, obviously, is not going to look like this - it will call only validateCheckAccessAndExecute() method, like it happens for DropIndexStatement now, without any "if" or so.

      Show
      Michał Michalski added a comment - Here's my idea of implementing this feature (see attached file for details): Move the validate(), checkAccess() and execute() method calls from QueryProcessor into separate "wrapping" method (I called it validateCheckAccessAndExecute for now ) defined in CQLStatement interface. All the classes implementing this interface will just execute these three methods one by one, like QueryProcessor did (a bit non-DRY). By doing so we can handle the exceptions thrown by these methods "internally" (in specific Statement classes), without adding any logic to QueryProcessor. Define two exceptions inheriting from InvalidRequestException so we can distinguish when we can (if IF [NOT] EXISTS was used in the query) just ignore a problem that occured - let's call them: MissingObject and ObjectExists. They are - obviously - thrown when we try to create existing object or drop the one that does not exist. Define two subclasses of SchemaAlteringStatement: DropStatement and CreateStatement - they require one additional parameter in constructor (indicating if IF [NOT] EXISTS statement was used) and override the validateCheckAccessAndExecute() method to check if any of the exceptions mentioned above was thrown and - if allowed - handle it by returning null (converted to Void message in QueryProcessor) or re-throw the exception. I've implemented this solution for DROP INDEX statement as an example - see attachment. If you're OK with this implementation, I'll do the rest. For now there are no changes in cqlsh (so autocompletion does not work), but of course it's also in the scope of this task. Important QueryProcessor code, obviously, is not going to look like this - it will call only validateCheckAccessAndExecute() method, like it happens for DropIndexStatement now, without any "if" or so.
      Gavin made changes -
      Workflow patch-available, re-open possible [ 12752853 ] reopen-resolved, no closed status, patch-avail, testing [ 12755589 ]
      Gavin made changes -
      Workflow no-reopen-closed, patch-avail [ 12615426 ] patch-available, re-open possible [ 12752853 ]
      Hide
      Jonathan Ellis added a comment -

      Also: CREATE IF NOT EXISTS.

      Show
      Jonathan Ellis added a comment - Also: CREATE IF NOT EXISTS.
      Jonathan Ellis made changes -
      Fix Version/s 2.0 [ 12322954 ]
      Jonathan Ellis made changes -
      Field Original Value New Value
      Issue Type Improvement [ 4 ] New Feature [ 2 ]
      Cathy Daw created issue -

        People

        • Assignee:
          Michał Michalski
          Reporter:
          Cathy Daw
          Reviewer:
          Sylvain Lebresne
        • Votes:
          2 Vote for this issue
          Watchers:
          7 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development