[
https://issues.apache.org/jira/browse/CASSANDRA-12373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16172976#comment-16172976
]
Sylvain Lebresne commented on CASSANDRA-12373:
----------------------------------------------
Thanks for the changes and you patience on this. My main remaining remark is
that I don't think we should include the SC key and value columns to
{{partitionColumns}} (in {{CFMetaData.rebuild}}).
{{PartitionColumns}} is meant and used for the columns of the internal storage
engine, but the SC key and value columns are "fake" columns used for the CQL
translation, they will never have values internally, and so should never reach
deep in the storage engine. Which means they shouldn't be in
{{PartitionColumns}}. In fact, I suspect that's why you needed to have special
code in {{Columns}} and {{SerializationHeader}}, which feels wrong because you
shouldn't ever encounter those definitions that deep in the storage engine.
Don't get me wrong, I'm sure there may be a few places in the CQL layers where
we rely on {{CFMetaData.partitionColumns()}} and need those columns, and that's
probably why you did that, but we imo need to identify those places and special
case them.
Related to this (because due to this), I think the change in
{{ColumnFamilyStoreCQLHelperTest}} is incorrect: it would be appropriate for
{{ColumnFamilyStoreCQLHelper}} to either display the storage schema (so no
"column2" nor "value"), or the CQL one (so no SCF empty-named map), but
something is between is not consistent. Anyway, mainly pointing that we really
need to remove those columns from {{partitionColumns}} and revert the change in
{{ColumnFamilyStoreCQLHelperTest}}.
Other than that, only a few minor remarks:
* In {{CFMetaData.renameColumn}}, in the case of updating the SC key or value
column, I believe we should be updating {{columnMetadata}} as well since those
columns are listed in it, but that doesn't seem to be the case (not sure how
important it is, it might be a following call to {{rebuild}} fixes that in
practice, but since the method doesn't call {{rebuild}} itself, probably better
to make sure we handle it).
* In {{CFMetaData.makeLegacyDefaultValidator}}, compact tables with counter
will now return {{BytesType}} instead of {{CounterColumnType}}, which is kind
of technically incorrect. To be entirely honest, this doesn't matter currently
because that method isn't ever called for non-compact tables (and at this
point, probably never will), but if we're going to rely on this, I'd rather
make it an assertion than returning something somewhat wrong. Personally, I'd
just keep the counter special case and move on, as this has nothing to do with
this ticket, but if you prefer transforming it to a {{assert
!isCompactTable()}}, no complain.
* Nit: in {{CFMetaData.renameColumn}}, the comment "SuperColumn tables allow
renaming all columns" doesn't match the code entirely anymore.
* Nit: in {{CassandraServer.makeColumnFilter}}, it would be more readable to
just cut the method short if {{metadata.isDense()}} before the loop, with maybe
a comment explaining why it's ok to do so ("Dense table only have dynamic
columns").
* Nit: in {{SuperColumnCompatibility.getSuperCfKeyColumn}}, I don't think the
"3.x created supercolumn family" comment is accurate anymore since in
{{ThriftConversion}} you now add the 2nd clustering column (which, in itself,
lgtm). It might be we need to preserve that branch in
{{SuperColumnCompatibility.getSuperCfKeyColumn}} for some upgrade path, and
happy to do so, but should update the comment.
> 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
