Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Tue, 12 Sep 2017 15:04:00 +0000 (UTC)
From: "Sylvain Lebresne (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12994614.1470237400000.91563.1505228640974@Atlassian.JIRA>
In-Reply-To: <JIRA.12994614.1470237400000@Atlassian.JIRA>
References: <JIRA.12994614.1470237400000@Atlassian.JIRA> <JIRA.12994614.1470237400679@jira-lw-us.apache.org>
Subject: [jira] [Commented] (CASSANDRA-12373) 3.0 breaks CQL compatibility
 with super columns families
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Tue, 12 Sep 2017 15:04:08 -0000


    [ 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