[
https://issues.apache.org/jira/browse/CASSANDRA-10124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14802988#comment-14802988
]
Sylvain Lebresne commented on CASSANDRA-10124:
----------------------------------------------
I don't have much on the substance, the patch lgtm. Maybe one point is that I
wonder if appending all the column names to build the default index name (which
btw will look weird for no-arg indexes) is the best strategy. I understand it's
a natural extension of our current strategy, but it feels like it could become
unwieldy, and we'll have to find a better strategy anyway when we have
functional index for instance. Anyway, not a big deal and I'd be fine going
with this but figure I'd at least raise the concern for discussion.
On the style however, I do want to bring one point and I somewhat apologize
because I realize that it partly enter the realm of personal preference, but I
feel that you over-abuse the Stream API and I'm not really a fan. Typically, I
don't see how:
{noformat}
List<IndexTarget> targets = rawTargets.stream()
.map(rawTarget -> rawTarget.prepare(cfm))
.collect(Collectors.toList());
{noformat}
is better than
{noformat}
List<IndexTarget> targets = new ArrayList<>(rawTargets.size());
for (IndexTarget.Raw rawTarget : rawTargets)
targets.add(rawTarget.prepare(cfm));
{noformat}
The latter is not significantly more characters, is more efficient (you don't
allocate a lambda and a stream, and you properly size the output collection
right away) and, at this point in time, is a lot more java idiomatic. And if
you're really looking for shorter, then guava's {{Lists.transform}} method is
kind of better:
{noformat}
List<IndexTarget> targets = Lists.transform(rawTargets, t -> t.prepare(cfm));
{noformat}
Or to take a imo more extreme example, I find
{{CreateIndexStatement.validateTargetsForMultiColumnIndex}} unreadable when we
could just do:
{noformat}
Set<ColumnIdentifier> columns = new HashSet<>();
for (IndexTarget target : targets)
{
if (!columns.add(target.column))
throw new InvalidRequestException("Duplicate column " + target.column +
" in index target list");
}
{noformat}
Granted, your version give the full list of duplicated columns, but I really
don't think we care.
Please don't get me wrong, I'm not saying we should never use the Stream API,
and some of the use in that very patch can be justified. But going from my last
reviews of you patches, you seem to use it as a default which, well, I'm not a
fan.
> Support for multi-column indexes
> --------------------------------
>
> Key: CASSANDRA-10124
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10124
> Project: Cassandra
> Issue Type: Bug
> Reporter: Sam Tunnicliffe
> Assignee: Sam Tunnicliffe
> Labels: client-impacting
> Fix For: 3.0.0 rc1
>
>
> Since CASSANDRA-6717 decoupled a secondary index from a single column, we can
> expand support for indexes with multiple target columns and for row-based
> indexes with truly dynamic targets.
> Much of the plumbing for this has been done in CASSANDRA-7771, CASSANDRA-6717
> & by the API rework in CASSANDRA-9459. What remains is:
> * Decide on syntax for DDL statements
> * Decide on WHERE clause syntax for SELECT (there is some discussion on this
> in the comments of CASSANDRA-9459)
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
