Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 0.8.1
    • Component/s: Core
    • Labels:
      None

      Description

      Being able to provide arguments to a comparator, to parametrize its behavior, can be useful.
      For example, a generic ReverseComparator is trivial to write, but without the ability to provide arguments at the comparator constriction, it cannot be used.
      CASSANDRA-2231 provides another use case for this.

      This ticket proposes to introduce a parser for comparator definition that supports optional parameters and to introduce a generic ReverseType as a simple example of this.

        Issue Links

          Activity

          Sylvain Lebresne created issue -
          Hide
          Ed Anuff added a comment -

          In Cassandra-2231 it was suggested that this be implemented by making comparators parameterizable. The idea would be to perhaps replace FBUtilities.getComparator() with a ComparatorFactory that could be passed something like "UUIDType(restrictTo=time,sort=desc)" and parse out the parameters in order to construct the instance. For Cassandra-2231, the proposed patch requires that FBUtilities.getComparator() caches and returns the same singleton comparator instances, so that requesting "UUIDType" will always return the same instance. It would be necessary to cache the parameterized version in a similar way, and would probably need to be able to know that "UUIDType(restrictTo=time,sort=desc)" and "UUIDType(sort=desc,restrictTo=time)" should return the same cached comparator.

          Show
          Ed Anuff added a comment - In Cassandra-2231 it was suggested that this be implemented by making comparators parameterizable. The idea would be to perhaps replace FBUtilities.getComparator() with a ComparatorFactory that could be passed something like "UUIDType(restrictTo=time,sort=desc)" and parse out the parameters in order to construct the instance. For Cassandra-2231, the proposed patch requires that FBUtilities.getComparator() caches and returns the same singleton comparator instances, so that requesting "UUIDType" will always return the same instance. It would be necessary to cache the parameterized version in a similar way, and would probably need to be able to know that "UUIDType(restrictTo=time,sort=desc)" and "UUIDType(sort=desc,restrictTo=time)" should return the same cached comparator.
          Sylvain Lebresne made changes -
          Field Original Value New Value
          Description Being able to provide arguments to a comparator, to parametrize its behavior, can be useful.
          For example, a generic ReverseComparator is trivial to write, but without the ability to provide arguments at the comparator constriction, it cannot be used.
          CASSANDRA-2231 provides another use case for this.

          This ticket proposes to introduce a parser for comparator definition that supports optional parameters and to introduce a generic ReverseType as a simple example of this.
          Summary Have an easy way to define the reverse comparator of any comparator Allow parameters for comparator
          Fix Version/s 0.7.5 [ 12316288 ]
          Hide
          Sylvain Lebresne added a comment -

          Attaching patch against 0.7.

          It may be important to keep in mind that once you use a comparator with arguments, your node become network incompatible with older nodes, since those older nodes won't be able to understand the comparator shipping in new schema migrations.

          Show
          Sylvain Lebresne added a comment - Attaching patch against 0.7. It may be important to keep in mind that once you use a comparator with arguments, your node become network incompatible with older nodes, since those older nodes won't be able to understand the comparator shipping in new schema migrations.
          Sylvain Lebresne made changes -
          Sylvain Lebresne made changes -
          Link This issue blocks CASSANDRA-2231 [ CASSANDRA-2231 ]
          Sylvain Lebresne made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Jonathan Ellis added a comment -

          Wouldn't this be more appropriate for 0.8 then?

          Show
          Jonathan Ellis added a comment - Wouldn't this be more appropriate for 0.8 then?
          Hide
          Jonathan Ellis added a comment -

          ... never mind, it's totally reasonable to say "don't create a comparator w/ this new feature until you've upgraded the entire cluster"

          Show
          Jonathan Ellis added a comment - ... never mind, it's totally reasonable to say "don't create a comparator w/ this new feature until you've upgraded the entire cluster"
          Hide
          Jonathan Ellis added a comment -

          Are there any other uses for comparator options?

          If not perhaps this approach is premature generalization.

          Show
          Jonathan Ellis added a comment - Are there any other uses for comparator options? If not perhaps this approach is premature generalization.
          Jonathan Ellis made changes -
          Fix Version/s 0.8 [ 12314820 ]
          Fix Version/s 0.7.5 [ 12316288 ]
          Hide
          Sylvain Lebresne added a comment -

          Are there any other uses for comparator options?

          The idea was to allow reversed comparator, and the composite types and more importantly both those together. In particular this improves slightly I believe the parsing for the composite types themselves. The fact that it is generic also makes use usable for custom comparator. There was also some idea of generalizing existing comparator hinted in the comment above. But for exemple, we could easily have a TextComparator(charset='whatever'), which could be cool. Or a TextComparator(regexp='...') who would validate only entry matching some regexp. Maybe that last one is not so useful, I don't know, but I do believe that it opens a bunch of interesting possibility.

          Show
          Sylvain Lebresne added a comment - Are there any other uses for comparator options? The idea was to allow reversed comparator, and the composite types and more importantly both those together. In particular this improves slightly I believe the parsing for the composite types themselves. The fact that it is generic also makes use usable for custom comparator. There was also some idea of generalizing existing comparator hinted in the comment above. But for exemple, we could easily have a TextComparator(charset='whatever'), which could be cool. Or a TextComparator(regexp='...') who would validate only entry matching some regexp. Maybe that last one is not so useful, I don't know, but I do believe that it opens a bunch of interesting possibility.
          Jonathan Ellis made changes -
          Fix Version/s 1.0 [ 12316349 ]
          Fix Version/s 0.8 [ 12314820 ]
          Hide
          Jonathan Ellis added a comment -

          +1

          Show
          Jonathan Ellis added a comment - +1
          Hide
          Sylvain Lebresne added a comment -

          Attached v3 (v3 is for coherence with CASSANDRA-2231) is rebased on 0.8.

          Show
          Sylvain Lebresne added a comment - Attached v3 (v3 is for coherence with CASSANDRA-2231 ) is rebased on 0.8.
          Sylvain Lebresne made changes -
          Sylvain Lebresne made changes -
          Fix Version/s 0.8 [ 12314820 ]
          Fix Version/s 1.0 [ 12316349 ]
          Sylvain Lebresne made changes -
          Issue Type Improvement [ 4 ] New Feature [ 2 ]
          Fix Version/s 0.8.1 [ 12316368 ]
          Fix Version/s 0.8 [ 12314820 ]
          Hide
          Sylvain Lebresne added a comment -

          Attaching companion patch for the 0.7 version of CASSANDRA-2231. See the comment there.

          Show
          Sylvain Lebresne added a comment - Attaching companion patch for the 0.7 version of CASSANDRA-2231 . See the comment there.
          Sylvain Lebresne made changes -
          Hide
          Sylvain Lebresne added a comment -

          Attaching v4 that is rebased against current trunk.

          Show
          Sylvain Lebresne added a comment - Attaching v4 that is rebased against current trunk.
          Sylvain Lebresne made changes -
          Hide
          Sylvain Lebresne added a comment -

          Committed to 0.8.1 based on jbellis +1

          Show
          Sylvain Lebresne added a comment - Committed to 0.8.1 based on jbellis +1
          Sylvain Lebresne made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Reviewer jbellis
          Resolution Fixed [ 1 ]
          Gavin made changes -
          Workflow no-reopen-closed, patch-avail [ 12608062 ] patch-available, re-open possible [ 12751363 ]
          Gavin made changes -
          Workflow patch-available, re-open possible [ 12751363 ] reopen-resolved, no closed status, patch-avail, testing [ 12754892 ]
          Gavin made changes -
          Link This issue blocks CASSANDRA-2231 [ CASSANDRA-2231 ]
          Gavin made changes -
          Link This issue is depended upon by CASSANDRA-2231 [ CASSANDRA-2231 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          2d 23h 29m 1 Sylvain Lebresne 21/Mar/11 14:22
          Patch Available Patch Available Resolved Resolved
          49d 3h 3m 1 Sylvain Lebresne 09/May/11 18:25

            People

            • Assignee:
              Sylvain Lebresne
              Reporter:
              Sylvain Lebresne
              Reviewer:
              Jonathan Ellis
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 4h
                4h
                Remaining:
                Remaining Estimate - 4h
                4h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development