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 Tue, 12 Sep 2017 15:04:00 GMT

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


Mime
View raw message