[
https://issues.apache.org/jira/browse/CASSANDRA-10092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14974081#comment-14974081
]
Sam Tunnicliffe commented on CASSANDRA-10092:
---------------------------------------------
Thanks for the updated patch, though I'm afraid there are a couple of issues
with it.
The validation in {{CassandraServer.createMutationList}} isn't really doing
what it's supposed to. It simply creates an empty columnfamily and passes that
to the validation method. This case isn't actually covered by the new tests
(but it would be good to do so), but even if it were, the implementation of the
new method in the test index impl would let it pass as the validation is based
solely on the key. So if you could change that validation & extend the test to
exercise each of the places where it's called (i.e. regular, batch and CAS
mutations), that'd be good.
The patch also doesn't merge cleanly to 2.2; there are some trivial merge
conflicts, but once those are resolved the several of the tests in
{{PerRowSecondaryIndexTest}} error. I suspect this is something to do with the
way {{testInvalidRowInsertThrift}} sets up {{Embedded CassandraService}}, you
might want to check out the other tests which use that without errors & verify
that they're equivalent.
> Generalize PerRowSecondaryIndex validation
> ------------------------------------------
>
> Key: CASSANDRA-10092
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10092
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Andrés de la Peña
> Assignee: Andrés de la Peña
> Priority: Minor
> Labels: 2i, secondary_index, validation
> Fix For: 2.1.x, 2.2.x
>
> Attachments: CASSANDRA-10092_v2.patch, improve_2i_validation.patch
>
>
> Index validation is currently done in a per-cell basis. However, per-row
> secondary index developers can be interested in validating all the written
> columns at once, because some implementations need to check the validity of a
> row write by comparing some column values against others. For example, a per
> row 2i implementation indexing time ranges (composed by a start date column
> and an end date column) should check that the start date is before the stop
> date.
> I'm attaching a patch adding a new method to {{PerRowSecondaryIndex}}:
> {code:java}
> public void validate(ByteBuffer key, ColumnFamily cf) throws
> InvalidRequestException {}
> {code}
> and a new method to {{SecondaryIndexManager}}:
> {code:java}
> public void validateRowLevelIndexes(ByteBuffer key, ColumnFamily cf) throws
> InvalidRequestException
> {
> for (SecondaryIndex index : rowLevelIndexMap.values())
> {
> ((PerRowSecondaryIndex) index).validate(key, cf);
> }
> }
> {code}
> This method is invoked in CQL {{UpdateStatement#validateIndexedColumns}}.
> This way, {{PerRowSecondaryIndex}} could perform complex write validation.
> I have tried to do the patch in the least invasive way possible. Maybe the
> current method {{SecondaryIndex#validate(ByteBuffer, Cell)}} should be moved
> to {{PerColumnSecondaryIndex}}, and the {{InvalidRequestException}} that
> informs about the particular 64k limitation should be thrown by
> {{AbstractSimplePerColumnSecondaryIndex}}. However, given the incoming
> [CASSANDRA-9459|https://issues.apache.org/jira/browse/CASSANDRA-9459], I
> think that the proposed patch is more than enough to provide rich validation
> features to 2i implementations based on 2.1.x and 2.2.x.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
