cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Samuel Klock (JIRA)" <>
Subject [jira] [Updated] (CASSANDRA-8473) Secondary index support for key-value pairs in CQL3 maps
Date Mon, 15 Dec 2014 06:27:13 GMT


Samuel Klock updated CASSANDRA-8473:
    Attachment: trunk-8473-v2.txt

Attaching new versions of the patch (for 2.1 and trunk) that are intended to address your
feedback.  Details:

* {{SingleColumnRelation}}
** bq. For lists, this error message may be confusing: {{checkTrue(receiver.type instanceof
MapType, "Column \"%s\" cannot be used as a map",;}}. For example, if you do
{{WHERE mylist\[0\] = 'foo'}}, you're not really trying to use it as a map. You may want to
handle lists specially.
Done: lists get a different error message now.  (In the 2.1 patch, this change is made in
** bq. The {{checkFalse()}} statement below this is starting to get confusing, I would break
it up
Agreed.  Refactored the condition into a couple of extracted methods.  (In the 2.1 patch,
a similar change is made in {{SelectStatement}}.)
** bq. Use curly braces on the "if" clause when they're used on the "else"
Done in trunk patch.  No equivalent issue in 2.1 patch.
** bq. I'm not sure that frozen maps are handled correctly here (e.g. WHERE myfrozenmap\['foo'\]
= 'bar'). May want to double-check that.
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.
* {{SingleColumnRelation.Contains()}}
** bq. Update class-level comment (should be a javadoc) to include map entry restrictions
Updated the comment in the trunk patch (it was already updated in the 2.1 version).  Making
it a Javadoc would be inconsistent with the other nested classes in {{SingleColumnRestriction}}
(which have no Javadoc), so I'm reluctant to do so.  But I wouldn't forcefully object.
** bq. entries(): no need for curly braces with a single-line for-loop
Moot because the for-loop is now multiline.  No equivalent issue in the 2.1 patch.
* {{CreateIndexStatement}}
** bq. switch on target.type could be clearer if re-organized; also, the error message about
'keys' is slightly misleading for 'entries' indexes
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?
* {{IndexTarget.TargetType.fromIndexOptions()}}
** bq. Should this return FULL if index_values isn't present? Also, no curlies needed for
single-line clauses.
Removed the braces.  On the return value: no index options will be present if the column isn't
a non-frozen collection.  Even so, this is a correctness issue: the v1 patch would yield the
wrong error message if a user attempted to create a {{FULL}} index on a frozen map that already
had one.  Fixed in v2.
* {{ExtendedFilter}}
** bq. {{else if (expr.isContains())}} will always be false (due to the {{isContains()}} check
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.)
* {{CompositesIndex}}
** bq. No nested ternaries, please
Rewritten as {{if}} statements.
* {{CompositesIndexOnCollectionKeyAndValue}}
** bq. makeIndexColumnPrefix(): need to use {{min(count - 1, cellName.size())}} for loop end
(see CASSANDRA-8053 for why)
Fixed by extending {{CompositesIndexOnCollectionKey}}.  (Also, did you mean CASSANDRA-8073?)
** bq. by extending CompositesIndexOnCollectionKey, you could eliminate about half of the
Done, although 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?).
 But the reduced duplication is very nice.
** bq. isStale(): instead of building a new composite to compare with the index entry key,
why not compare the cell value with the second item in the index entry composite? This method
could also use a comment or two
Good point -- the implementation no longer builds a new composite for the comparison.  I've
also refactored the code for clarity; if you still think it needs comments, I'll certainly
add them.  (Also: fixed the return value for when the cell in the indexed table is dead.)
* {{SecondaryIndexOnMapEntriesTest}}
** bq. Unusued and commented out imports
Removed the commented imports.  I don't think any were unused.
** bq. Add a test for {{map\[element\] = null}} being invalid. (While we could support this
when filtering, we couldn't support it with a 2ary index lookup.)
Added a test and made it pass (these queries would fail with NPEs).  It's worth noting that
the null check for map entries occurs in a different place than that for {{CONTAINS}}/{{CONTAINS
KEY}} relations for implementation reasons (the keys for index entries are structured differently).
 I'd be interested if anyone has an opinion on how to do this better.

> Secondary index support for key-value pairs in CQL3 maps
> --------------------------------------------------------
>                 Key: CASSANDRA-8473
>                 URL:
>             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,
> 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':
> cqlsh:mykeyspace> INSERT INTO mytable (key, value) VALUES ('baz', {'b': '4', 'c':
> 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'
>  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

View raw message