[
https://issues.apache.org/jira/browse/CASSANDRA-8473?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Samuel Klock updated CASSANDRA-8473:
------------------------------------
Attachment: trunk-8473-v2.txt
cassandra-2.1-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", receiver.name);}}. 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 {{SelectStatement}}.)
** 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 above).
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 methods
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: 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)
