[ https://issues.apache.org/jira/browse/CASSANDRA-12373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16166050#comment-16166050 ]
Alex Petrov commented on CASSANDRA-12373: ----------------------------------------- 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: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org