[
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)
