Details

      Description

      The CQL3 "create index" statement syntax does not allow to specify the options map internally used by custom indexes.

      1. 6480-v2.txt
        10 kB
        Aleksey Yeschenko
      2. 6480-v3.txt
        11 kB
        Aleksey Yeschenko

        Activity

        Hide
        slebresne Sylvain Lebresne added a comment -

        Out of curiosity, do you actually have a use case for it? Asking because we left it out of CASSANDRA-5484 for lack of being aware of any real life use case.

        Show
        slebresne Sylvain Lebresne added a comment - Out of curiosity, do you actually have a use case for it? Asking because we left it out of CASSANDRA-5484 for lack of being aware of any real life use case.
        Hide
        adelapena Andrés de la Peña added a comment -

        Hi Sylvain,
        yes, we have an use case. We're developing a custom secondary index based on Lucene, it would be nice to able to have a syntax similar to the 'create keyspace' statement, in which we can pass the complete index options map (which already exists in the code). For example:

        CREATE TABLE documents (
        key timeuuid,
        date timestamp,
        spanish_text varchar,
        english_text varchar,
        PRIMARY KEY key);

        CREATE CUSTOM INDEX ON demo.users (spanish_text) USING

        {'class' : 'org.stratio.FullTextIndex', 'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'}

        ;
        CREATE CUSTOM INDEX ON demo.users (english_text) USING

        {'class' : 'org.stratio.FullTextIndex', 'analyzer': 'EnglishAnalyzer', 'storage':'/mnt/ssd/indexes/'}

        ;

        Show
        adelapena Andrés de la Peña added a comment - Hi Sylvain, yes, we have an use case. We're developing a custom secondary index based on Lucene, it would be nice to able to have a syntax similar to the 'create keyspace' statement, in which we can pass the complete index options map (which already exists in the code). For example: CREATE TABLE documents ( key timeuuid, date timestamp, spanish_text varchar, english_text varchar, PRIMARY KEY key); CREATE CUSTOM INDEX ON demo.users (spanish_text) USING {'class' : 'org.stratio.FullTextIndex', 'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'} ; CREATE CUSTOM INDEX ON demo.users (english_text) USING {'class' : 'org.stratio.FullTextIndex', 'analyzer': 'EnglishAnalyzer', 'storage':'/mnt/ssd/indexes/'} ;
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        As Sylvain says, it's been left out b/c there was no use case for it. I was going to add WITH OPTIONS =

        {..}

        or something like it for custom 2i simultaneously with CASSANDRA-5962 for 2.1, but for 2i alone it can go into 2.0.

        Show
        iamaleksey Aleksey Yeschenko added a comment - As Sylvain says, it's been left out b/c there was no use case for it. I was going to add WITH OPTIONS = {..} or something like it for custom 2i simultaneously with CASSANDRA-5962 for 2.1, but for 2i alone it can go into 2.0.
        Hide
        slebresne Sylvain Lebresne added a comment -

        for 2i alone it can go into 2.0

        Pretty sure we explicitly don't want to support options for non-custom 2i because the only options they support should actually not be messed with and there is no reason to let user shoot themselves in the foot. More precisely, we may someday add options to non-custom 2i that it would make sense to expose to the user, but we don't have such options currently and letting the user change the ones currently used internally would be a bad idea, so it's probably simpler to just refuse options altogether for no-custom 2i for the time being.

        Show
        slebresne Sylvain Lebresne added a comment - for 2i alone it can go into 2.0 Pretty sure we explicitly don't want to support options for non-custom 2i because the only options they support should actually not be messed with and there is no reason to let user shoot themselves in the foot. More precisely, we may someday add options to non-custom 2i that it would make sense to expose to the user, but we don't have such options currently and letting the user change the ones currently used internally would be a bad idea, so it's probably simpler to just refuse options altogether for no-custom 2i for the time being.
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        Oh, definitely not for non-custom 2i. That would be a bad idea.

        Only talking about CREATE CUSTOM INDEX here.

        Show
        iamaleksey Aleksey Yeschenko added a comment - Oh, definitely not for non-custom 2i. That would be a bad idea. Only talking about CREATE CUSTOM INDEX here.
        Hide
        slebresne Sylvain Lebresne added a comment -

        Got it, I misunderstood the "for 2i alone" as meaning non-custom 2i rather that "for 2i but not for triggers". We're in agreement, my bad

        Show
        slebresne Sylvain Lebresne added a comment - Got it, I misunderstood the "for 2i alone" as meaning non-custom 2i rather that "for 2i but not for triggers". We're in agreement, my bad
        Hide
        adelapena Andrés de la Peña added a comment -

        Perfect, then... what is our opinion about options in custom 2i?
        Is everybdoy agreed about CQL3 shouldn't hide this useful feature on custom 2i?

        Show
        adelapena Andrés de la Peña added a comment - Perfect, then... what is our opinion about options in custom 2i? Is everybdoy agreed about CQL3 shouldn't hide this useful feature on custom 2i?
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        It's not about hiding, it's about YAGNI. If you attach a patch, we'll review it. If you don't, this will have to wait until other, more important things, are resolved.

        Show
        iamaleksey Aleksey Yeschenko added a comment - It's not about hiding, it's about YAGNI. If you attach a patch, we'll review it. If you don't, this will have to wait until other, more important things, are resolved.
        Hide
        adelapena Andrés de la Peña added a comment -

        https://github.com/Stratio/cassandra/commit/40ea80da906aaa5ec752fdb036fb4e9de7249409 https://github.com/Stratio/cassandra/commit/40ea80da906aaa5ec752fdb036fb4e9de7249409.patch

        We have changed the CREATE CUSTOM INDEX syntax to:

        CREATE CUSTOM INDEX ON users (spanish_text) USING 'org.stratio.FullTextIndex' WITH
        {'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'};
        

        Furthermore, it also accepts:

        CREATE CUSTOM INDEX ON users (spanish_text) WITH 
        {'class_name' : 'org.stratio.FullTextIndex', 'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'};
        

        And if no options are required:

        CREATE CUSTOM INDEX ON users (spanish_text) USING 'org.stratio.FullTextIndex';
        

        We have tried to maintain compatibility with the current syntax.

        Show
        adelapena Andrés de la Peña added a comment - https://github.com/Stratio/cassandra/commit/40ea80da906aaa5ec752fdb036fb4e9de7249409 https://github.com/Stratio/cassandra/commit/40ea80da906aaa5ec752fdb036fb4e9de7249409.patch We have changed the CREATE CUSTOM INDEX syntax to: CREATE CUSTOM INDEX ON users (spanish_text) USING 'org.stratio.FullTextIndex' WITH {'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'}; Furthermore, it also accepts: CREATE CUSTOM INDEX ON users (spanish_text) WITH {'class_name' : 'org.stratio.FullTextIndex', 'analyzer': 'SpanishAnalyzer', 'storage':'/mnt/ssd/indexes/'}; And if no options are required: CREATE CUSTOM INDEX ON users (spanish_text) USING 'org.stratio.FullTextIndex'; We have tried to maintain compatibility with the current syntax.
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        Attaching a v2, that handles this issue in a more C*-idiomatic way.

        Show
        iamaleksey Aleksey Yeschenko added a comment - Attaching a v2, that handles this issue in a more C*-idiomatic way.
        Hide
        slebresne Sylvain Lebresne added a comment -

        Minor remarks on the validation:

        • I'd reject 'class_name' as an option, instead of silently overriding.
        • I'd rather let non-custom CREATE INDEX with options pass the parser but be rejected later. An antlr parse error is a lot more confusing that a clear message telling you that options are not supported for non-custom indexes (I know there is existing places where we do something similar, but there's no reason not to improve our ways ).

        Nit: I'd move the isCustom and customClass fields inside IndexPropDefs.

        Show
        slebresne Sylvain Lebresne added a comment - Minor remarks on the validation: I'd reject 'class_name' as an option, instead of silently overriding. I'd rather let non-custom CREATE INDEX with options pass the parser but be rejected later. An antlr parse error is a lot more confusing that a clear message telling you that options are not supported for non-custom indexes (I know there is existing places where we do something similar, but there's no reason not to improve our ways ). Nit: I'd move the isCustom and customClass fields inside IndexPropDefs.
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        Attaching the updated v3.

        Show
        iamaleksey Aleksey Yeschenko added a comment - Attaching the updated v3.
        Hide
        slebresne Sylvain Lebresne added a comment -

        +1

        Show
        slebresne Sylvain Lebresne added a comment - +1
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        Committed, thanks.

        Show
        iamaleksey Aleksey Yeschenko added a comment - Committed, thanks.

          People

          • Assignee:
            iamaleksey Aleksey Yeschenko
            Reporter:
            adelapena Andrés de la Peña
            Reviewer:
            Sylvain Lebresne
          • Votes:
            2 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development