[
https://issues.apache.org/jira/browse/CASSANDRA-12373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16166050#comment-16166050
]
Alex Petrov edited comment on CASSANDRA-12373 at 9/14/17 3:22 PM:
------------------------------------------------------------------
Thank you for the review and comments.
I agree that having {{column2}} as clustering is better. I've tried to move
most of the special-casing to {{rebuild}} and {{SuperColumnCompatibility}}. I
think the patch got a bit cleaner thanks to that.
bq. 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.
Fixed and added corresponding tests to both 3.0 and 2.2
bq. 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().
I _think_ it can be simplified even further, since {{isCounter}} case will work
is because of the {{compactValueColumn}} (or map value type) and {{isCompact}}
call seems to be redundant alltogether.
bq. 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.
Fixed and added a couple more negative tests.
bq. 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.
This is still a bit of a problem, although just in one case. When upgrade was
done through unpatched 3.x, we end up without {{column2}} and {{value}}
columns. Now, we try renaming {{column1}} to {{column1_renamed}}, and, because
{{column2}} is still "virtual" (since it was not renamed), we may end up with
{{column2}} being called {{column1}} because of the defaults without this line.
I'd like to point out that renaming {{column2}} to {{column2}} and {{value}}
are not allowed even in that case (since all the columns are now in column
metadata map).
bq. 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?
Improved comments in header and for this inner class.
bq. for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1)
but it currently uses a >.
Fixed
bq. 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).
I was under impression that {{ColumnFilter.Builder#select}} allows just a
single collection constraint. Thanks for catching that. You're right, we can
handle it without any filtering, looks much better now.
bq. Nit: there is a few typo in those comments ("prece*e*ding" instead of
"preceding", "exlusive", "enitre", "... in this case since, since ...").
Fixed these and spell-checked to catch a couple more.
bq. 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.
Fixed.
bq. In Relation, I'd extend the error message to something like "Unsupported
operation (" + this + ") on super column family".
Fixed.
bq. Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations
could use brackets
Fixed this an several other ones.
bq. In MultiColumnRestriction.SliceRestriction, if my IDE don't fool me, it
appears we don't need to make slice public anymore.
You're right. Fixed.
bq. In StatementRestrictions, a few added imports are not needed (including the
NotImplementedException one).
Fixed this and several other cases.
|[3.0
patch|https://github.com/apache/cassandra/compare/cassandra-3.0...ifesdjeen:12373-3.0]|[3.11
patch|https://github.com/apache/cassandra/compare/cassandra-3.11...ifesdjeen:12373-3.11]|[dtests|https://github.com/apache/cassandra-dtest/compare/master...ifesdjeen:12373]|[2.2
patch|https://github.com/apache/cassandra/compare/cassandra-2.2...ifesdjeen:12373-2.2]|
was (Author: ifesdjeen):
Thank you for the review and comments.
I agree that having {{column2}} as clustering is better. I've tried to move
most of the special-casing to {{rebuild}} and {{SuperColumnCompatibility}}. I
think the patch got a bit cleaner thanks to that.
bq. 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.
Fixed and added corresponding tests to both 3.0 and 2.2
bq. 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().
I _think_ it can be simplified even further, since {{isCounter}} case will work
is because of the {{compactValueColumn}} (or map value type) and {{isCompact}}
call seems to be redundant alltogether.
bq. 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.
Fixed and added a couple more negative tests.
bq. 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.
This is still a bit of a problem, although just in one case. When upgrade was
done through unpatched 3.x, we end up without {{column2}} and {{value}}
columns. Now, we try renaming {{column1}} to {{column1_renamed}}, and, because
{{column2}} is still "virtual" (since it was not renamed), we may end up with
{{column2}} being called {{column1}} because of the defaults without this line.
I'd like to point out that renaming {{column2}} to {{column2}} and {{value}}
are not allowed even in that case (since all the columns are now in column
metadata map).
bq. 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?
Improved comments in header and for this inner class.
bq. for mutliEQRestriction should be ... AND (column1, column2) = ('value1', 1)
but it currently uses a >.
Fixed
bq. 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).
I was under impression that {{ColumnFilter.Builder#select}} allows just a
single collection constraint. Thanks for catching that. You're right, we can
handle it without any filtering, looks much better now.
bq. Nit: there is a few typo in those comments ("prece*e*ding" instead of
"preceding", "exlusive", "enitre", "... in this case since, since ...").
Fixed these and spell-checked to catch a couple more.
bq. 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.
Fixed.
bq. In Relation, I'd extend the error message to something like "Unsupported
operation (" + this + ") on super column family".
Fixed.
bq. Last else of 2nd loop in SuperColumnCompatibility.prepareUpdateOperations
could use brackets
Fixed this an several other ones.
bq. In MultiColumnRestriction.SliceRestriction, if my IDE don't fool me, it
appears we don't need to make slice public anymore.
You're right. Fixed.
bq. In StatementRestrictions, a few added imports are not needed (including the
NotImplementedException one).
Fixed this and several other cases.
Rebased and pushed to the [same
branch|https://github.com/apache/cassandra/compare/cassandra-3.0...ifesdjeen:12373-3.0].
Since it seems that we're getting closer to the end, I'll rebase {{3.11}} and
backport tests to 2.x.
> 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: [email protected]
For additional commands, e-mail: [email protected]
