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

          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.
          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.
          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.
          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.
          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.
          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.
          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.
          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

            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