[
https://issues.apache.org/jira/browse/CASSANDRA-10216?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14743256#comment-14743256
]
Sylvain Lebresne commented on CASSANDRA-10216:
----------------------------------------------
My main remark is that for the new {{target}} option, I'd prefer keeping it as
close to the {{CREATE INDEX}} definition as possible. So if it's not a
collection index, I'd prefer the "target" to just be the column name instead of
defaulting to {{"values(name)"}}. My main reasoning is that it gives an ever
simpler match between the schema table and the corresponding statement. But it
also feels somewhat cleaner more future-proof to me (and "values" is plural,
which sounds weird for a non-collection column). And updating the regexp to
match that doesn't seem too hard.
A few smaller points/nits:
* In {{Index.java}}, maybe {{s/requiresColumn/dependsOn/}} since we have
{{dependentIndexes()}} (may want to update javadoc accordingly).
* In {{SecondaryIndexManager.getDependentIndexes()}}, not super fan of using
lambdas: replacing the {{forEach}} call by a for-loop would feel almost more
readable to me because more idiomatic (plus while one could hope that lambda
creation is optimized out, I'm not all that sure). In fact, that's arguably a
personal preference, but I'd advocate staying away from {{forEach}} if we're
gonna construct the lamdba inside the call (for the reasons above: it probably
has a small cost and I don't find that it improve readability).
* In {{CassandraIndex.parseTarget}}, in the {{getColumnDefinition}}, you want
to use {{new ColumnIdendifier}} over {{ColumnIdentifier.getInterned}}: this is
a lookup, so if you we get a hit, the returned {{ColumnDefinition}} will use a
properly interned {{ColumnIdentifier}}, but if we don't (which can't happen
here), that could be a bogus name and we don't want to intern it.
* In {{CassandraIndex.parseTarget}}, would be nice to preserve the comment
explaining why non-CQL tables are handled differently (because we can't
guarantee the column name is UTF8 upfront).
* Nit: slightly buggy indentations in around {{CassandraIndex.parseTarget}}:
both the method 2nd parameter and the next method ({{getFunctions}})
declaration are mis-aligned.
* In {{IndexMetadata}}, it's weird to have both a {{singleTargetIndex}} and a
{{SingleColumnIndex}}. Can we rename the 2nd one (or, probably even better, get
rid of it and deal with that in whatever test is using it by using a small
helper)?
> Remove target type from internal index metadata
> -----------------------------------------------
>
> Key: CASSANDRA-10216
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10216
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sam Tunnicliffe
> Assignee: Sam Tunnicliffe
> Labels: client-impacting
> Fix For: 3.0.0 rc1
>
>
> As part of CASSANDRA-6716 & in anticipation of CASSANDRA-10124, a distinction
> was introduced between secondary indexes which target a fixed set of 1 or
> more columns in the base data, and those which are agnostic to the structure
> of the underlying rows. This distinction is manifested in
> {{IndexMetadata.targetType}} and {{system_schema.indexes}}, in the
> {{target_type}} column. It could be argued that this distinction complicates
> the codebase without providing any tangible benefit, given that the target
> type is not actually used anywhere.
> It's only the impact on {{system_schema.indexes}} that makes puts this on the
> critical path for 3.0, any code changes are just implementation details.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
