[
https://issues.apache.org/jira/browse/CASSANDRA-12373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16163085#comment-16163085
]
Sylvain Lebresne commented on CASSANDRA-12373:
----------------------------------------------
The one main remaining things I'm not sure about is that it seems possible to
have different schema (meaning, content of schema tables) for what is
essentially the same table, depending on how it was created/upgraded. Namely,
it appears a dense SCF may have 1 or 2 clustering and may or may not have
definitions for the so-called super column "key" and "values" columns.
This makes it hard, at least to me, to reason about things and have confidence
it always work as expected. This also feels error prone in the future.
Typically, most code is written expecting that
{{CFMetaData.primaryKeyColumns()}} would always be equals to
{{CFMetaData.partitionKeyColumns() + CFMetaData.clusteringColumns()}}, but
that's not necessarilly the case here for SCF (and whether it's the case or not
really depend more on how the table was created that the table definition).
Note that I'm not saying this particular example is a problem today, I believe
it's not, but I'm worried about how fragile this feel.
So my preference would be to force things to be more consistent. What I mean
here is that I would make it so that:
* in the schema tables, every dense SCF table has 2 {{CLUSTERING}} (the 1st
"true" clustering, and the 2nd standing for the SC "key" column) and 2
{{REGULAR}} definition (the SC "map" and the SC "value" column). Note that I
think it's important we save the "key" column as a {{CLUSTERING}} one:
otherwise, if both the "key" and "value" column definions are {{REGULAR}} (as I
think they can be in the current patch), you can't distinguish which is which
later one (and I think that's a current bug of
{{SuperColumnCompatibility.getSuperCfKeyColumn}}).
* but at the level of {{CFMetaData}}, we extract the "key" and "value" column
to their respective field, but otherwise remove them from {{clusteringColumns}}
and {{partitionColumns}}.
Other than, a bunch of other largely minor issues:
* In {{CFMetaData.renameColumn()}}, we appear to allow renaming every column
for any SCF, including non-dense ones. I don't think that was allowed in 2.x
(renaming non-PK columns of non-dense SCF through CQL) and I suggest
maintaining non supporting it. In fact, I don't think it's entirely safe in
some complex case of users still using thrift and doing schema-changes from it.
* I don't think the change in {{CFMetaData.makeLegacyDefaultValidator}} is
correct. That said, I don't think the previous code was correct either. If I'm
not mistaken, what we should be returning in the SCF case is
{{((MapType)compactValueColumn().type).valueComparator()}}.
* In {{SuperColumnCompatibility.prepareUpdateOperations}}, after the first
loop, I think we should check that {{superColumnKey != null}} (and provide a
meaningful error message if that's not the case). I believe otherwise we might
NPE when handling the {{Operation}}s created.
* In {{SuperColumnCompatibility.columnNameGenerator}}, I'm not sure I fully
understand the reason for always excluding {{"column1"}} (despite the comment).
Not that it's really a big deal.
* In {{SuperColumnCompatiblity.SuperColumnRestrictions}}, regarding the
different javadoc:
** for the class javadoc, since things are tricky, when saying "the default
column names are used", I think that's a good place to remind what "column1"
and "column2" means, and that both in term of the internal representation, of
their CQL exposure, and of the thrift correspondance. Or maybe move such
explanation to the {{SuperColumnCompatibility}} class javadoc and point to it?
** for {{mutliEQRestriction}} should be {{... AND (column1, column2) =
('value1', 1)}} but it currently uses a {{>}}.
** for {{keyInRestriction}}, the "This operation does _not_ have a direct
Thrift counterpart" isn't true. And In fact, I'm not sure why we have to fetch
everything and filter: can't we just handle this in {{getColumnFilter}} by only
selecting the map entries we want? Note that the one operation that does not
have a Thrift counterpart is {{mutliSliceRestriction}} (and, technically,
anything operation on strict bounds since Thrift was always inclusive).
** for {{keyEQRestriction}}, I believe "in `getRowFilter`" is supposed to be
"in `getColumnFilter`". Using a "\{@link\}" probably wouldn't hurt either :).
** Nit: there is a few typo in those comments ("prece*e*ding" instead of
"preceding", "exlusive", "enitre", "... in this case since, since ...").
And a few nitpicks:
* in {{MultiColumnRelation}}, both methods have {{List<ColumnDefinition>
receivers = receivers(cfm)}}, but then in the next line, they call
{{receivers(cfm)}} instead of just reusing {{receivers}}.
* In {{Relation}}, I'd extend the error message to something like
{{"Unsupported operation (" + this + ") on super column family"}}.
* Last {{else}} of 2nd loop in
{{SuperColumnCompatibility.prepareUpdateOperations}} could use brackets :)
* In {{MultiColumnRestriction.SliceRestriction}}, if my IDE don't fool me, it
appears we don't need to make {{slice}} public anymore.
* In {{StatementRestrictions}}, a few added imports are not needed (including
the {{NotImplementedException}} one).
> 3.0 breaks CQL compatibility with super columns families
> --------------------------------------------------------
>
> Key: CASSANDRA-12373
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12373
> Project: Cassandra
> Issue Type: Bug
> Components: CQL
> Reporter: Sylvain Lebresne
> Assignee: Alex Petrov
> Fix For: 3.0.x, 3.11.x
>
>
> This is a follow-up to CASSANDRA-12335 to fix the CQL side of super column
> compatibility.
> The details and a proposed solution can be found in the comments of
> CASSANDRA-12335 but the crux of the issue is that super column famillies show
> up differently in CQL in 3.0.x/3.x compared to 2.x, hence breaking backward
> compatibilty.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
