cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-12373) 3.0 breaks CQL compatibility with super columns families
Date Wed, 20 Sep 2017 10:17:00 GMT

    [ 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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


Mime
View raw message