cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tyler Hobbs (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-8473) Secondary index support for key-value pairs in CQL3 maps
Date Tue, 16 Dec 2014 20:26:14 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-8473?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14248854#comment-14248854
] 

Tyler Hobbs commented on CASSANDRA-8473:
----------------------------------------

Thanks for the quick turnaround on the patch.  Most of your changes and comments are good,
I'll just comment on a few things here:

bq. Added a test. I think trunk is okay (although the error message is imperfect: "Invalid
STRING constant (foo) for "myfrozenmap" of type frozen<map<text, text>>"). The
2.1 patch incorrectly gave no error for queries with this condition; that's been fixed.

Hmm, that error message indicates that the logic for frozen collections in {{toReceivers()}}
still isn't quite correct.  I think it needs to specifically check for map-entry relations
on frozen collections.  Right now, it's falling through to the default behavior of {{return
Collections.singletonList(receiver);}}, so it's expecting a full collection as the value in
the relation.

bq. The block has been removed. Note that it was an attempt to preserve the logic that existed
in the MAP case previously if expr.isContainsKey() were false; since the only kind of expressions
apart from CONTAINS KEY that were valid for map columns were CONTAINS relations, this seemed
like the right choice. But your analysis appears to be correct. Is there some other way that
code would have been reachable? (It looks like the code may have been intended to check map\[key\]
= value conditions, but AFAIK there would have been no way to trigger its execution.)

You're right.  Thinking about this method a little more, the only things that should be handled
after the {{isContains()}} check should be CONTAINS KEY and {{map\[key\] = value}}.  Frozen
collections aren't tested with this method, and lists and sets only support CONTAINS.  So
you could go ahead and simplify this whole method to reflect that.  (I think I may have failed
to remove old code here when I added support for frozen collections.)

bq. Corrected both error messages. It's not obvious to me how to reorganize the switch to
make it clearer (very likely because I wrote it) – did you have something specific in mind?

I think this could replace the switch statement (although I haven't tested it):
{code}
        if (isFrozenCollection)
        {
            if (target.type != IndexTarget.TargetType.FULL)
                throw new InvalidRequestException("Frozen collections currently only support
full-collection indexes. " +
                                                  "For example, 'CREATE INDEX ON <table>(full(<columnName>))'.");
        }
        else
        {
            if (target.type == IndexTarget.TargetType.FULL)
                throw new InvalidRequestException("full() indexes can only be created on frozen
collections");

            if (!cd.type.isMultiCell())
                throw new InvalidRequestException(String.format("Cannot create index on %s
of column %s; only non-frozen collections support %s indexes",
                                                                target.type, target.column,
target.type));

            if (target.type == IndexTarget.TargetType.KEYS || target.type == IndexTarget.TargetType.KEYS_AND_VALUES)
            {
                if (!isMap)
                    throw new InvalidRequestException(String.format("Cannot create index on
%s of column %s with non-map type",
                            target.type, target.column));
            }
        }
{code}

bq.  I've also refactored the code for clarity; if you still think it needs comments, I'll
certainly add them.

It looks perfectly clear as-is, thanks.

bq. I'm not positive the abstractions are quite right (is CompositesIndexOnCollectionKeyAndValue
really a kind of CompositesIndexOnCollectionKey? Would it be better to use a common superclass?

I would be okay with a common abstract superclass.  I agree that the abstraction doesn't line
up 100%.

Regarding 2.1, I understand your desire to not have to run a patched version of C*, but we
have to weigh that against potentially destabilizing 2.1 for other users.  My preference is
still to stick with a 3.0 target.  Maybe [~slebresne] can weigh in here?

> Secondary index support for key-value pairs in CQL3 maps
> --------------------------------------------------------
>
>                 Key: CASSANDRA-8473
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-8473
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Samuel Klock
>            Assignee: Samuel Klock
>             Fix For: 3.0
>
>         Attachments: cassandra-2.1-8473-actual-v1.txt, cassandra-2.1-8473-v2.txt, cassandra-2.1-8473.txt,
trunk-8473-v2.txt
>
>
> CASSANDRA-4511 and CASSANDRA-6383 made substantial progress on secondary indexes on CQL3
maps, but support for a natural use case is still missing: queries to find rows with map columns
containing some key-value pair.  For example (from a comment on CASSANDRA-4511):
> {code:sql}
> SELECT * FROM main.users WHERE notify['email'] = true;
> {code}
> Cassandra should add support for this kind of index.  One option is to expose a CQL interface
like the following:
> * Creating an index:
> {code:sql}
> cqlsh:mykeyspace> CREATE TABLE mytable (key TEXT PRIMARY KEY, value MAP<TEXT, TEXT>);
> cqlsh:mykeyspace> CREATE INDEX ON mytable(ENTRIES(value));
> {code}
> * Querying the index:
> {code:sql}
> cqlsh:mykeyspace> INSERT INTO mytable (key, value) VALUES ('foo', {'a': '1', 'b':
'2', 'c': '3'});
> cqlsh:mykeyspace> INSERT INTO mytable (key, value) VALUES ('bar', {'a': '1', 'b':
'4'});
> cqlsh:mykeyspace> INSERT INTO mytable (key, value) VALUES ('baz', {'b': '4', 'c':
'3'});
> cqlsh:mykeyspace> SELECT * FROM mytable WHERE value['a'] = '1';
>  key | value
> -----+--------------------------------
>  bar |           {'a': '1', 'b': '4'}
>  foo | {'a': '1', 'b': '2', 'c': '3'}
> (2 rows)
> cqlsh:mykeyspace> SELECT * FROM mytable WHERE value['a'] = '1' AND value['b'] = '2'
ALLOW FILTERING;
>  key | value
> -----+--------------------------------
>  foo | {'a': '1', 'b': '2', 'c': '3'}
> (1 rows)
> cqlsh:mykeyspace> SELECT * FROM mytable WHERE value['b'] = '2' ALLOW FILTERING;
>  key | value                         
> -----+--------------------------------
>  foo | {'a': '1', 'b': '2', 'c': '3'}
> (1 rows)                             
> cqlsh:mykeyspace> SELECT * FROM mytable WHERE value['b'] = '4';
>  key | value
> -----+----------------------
>  bar | {'a': '1', 'b': '4'}
>  baz | {'b': '4', 'c': '3'}
> (2 rows)
> {code}
> A patch against the Cassandra-2.1 branch that implements this interface will be attached
to this issue shortly.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message