[
https://issues.apache.org/jira/browse/CASSANDRA-12373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16153401#comment-16153401
]
Sylvain Lebresne commented on CASSANDRA-12373:
----------------------------------------------
Thanks for the update and great job on the much needed additional testing. I
think we're getting there but I still have a bunch of remarks :)
* In {{CFMetaData}}
** In the ctor, for {{isDense}}, we seem to force recomputation for non-SC
non-dense tables (which happens to include non-compact tables). I don't think
thats what we want (and the {{recalculateIsDense}} is obviously meant only for
SC). But for I'm also not 100% sure this work in all upgrade scenario.
Typically, if a dense SCF is upgraded to a current 3.x version, I believe it
will end up with 2 regular column definition: the first coming from the fact
2.x will have had a "COMPACT_VALUE" column definition, which will become a
REGULAR when upgraded, and second being the "super column map" column that is
force-added to all SCF. And so that re-computation code will not detect things
properly. Note that I haven't actually tested this tbh so I could be wrong, but
this highlight that we really need to be running some SC upgrade dtest both for
{{2.x \-> 3.x+<this patch>}} and {{2.x \-> current 3.x \-> 3.x+<this patch>}}
because things are imo too complex to rely on reasoning alone. Unfortunately,
while we were supposed to have a [SC upgrade
dtests|https://github.com/apache/cassandra-dtest/blob/master/upgrade_tests/upgrade_supercolumns_test.py],
it appears [this
commit|https://github.com/apache/cassandra-dtest/commit/7f47d4d82d6becdb9fd267bd3844d32aee6acdea]
has made this test useless by making it (confusingly) not about super column
families. So we'll need to revert that commit somehow and the improve the test
to include both dense and sparse SC.
** In {{rebuild()}}, why not move the SC code after {{columnMetadata}} has been
populated so it can be simplified? Nothing seems to rely on it being done
_before_ {{columnMetadata}} is populated.
* I'm not entirely sure if/how column renaming works for the SC key and value
"fake" column (would have assumed {{AlterTableStatement}} would need a least a
bit of special casing to recognize those "fake" column). It would be nice to
add at test to check that renames still work like in 2.2 for those columns.
* In {{UpdateParameters#addCounter}}, not sure the change from
{{CounterContext#createUpdate}} to {{CounterContext#createLocal}} is corret.
Probably a bad post-CASSANDRA-13691 rebase?
* In {{SuperColumnCompatibility}:
** The first line of the javadoc states that it's only for "dense" SC, but it's
not 100% true as things like {{SUPER_COLUMN_MAP_COLUMN}} are used for all SC.
And the rest of the javadoc describing the internal layout also applies to all
SC. Bit of a detail I suppose, but I'd rephrase things to avoid any confusion.
** In {{getSuperCfKeyColumn}}, not sure what "3.x-created supercolumn family"
refers to (is that SC tables created from thrift in 3.x? or SC tables whose
schema had been upgraded by a previous 3.x version?) and more importantly, why
it would differ in number of clustering columns?
** I'm confused by the code of {{getSuperCfValueColumn}}: dense tables are
supposed to have only a single regular column, and {{cfm.compactValueColumn()}}
is supposed to alias it. So I'm not sure what the loop is trying to do, except
basically returning {{cfm.compactValueColumn()}}, nor when that loop wouldn't
return (all SC should have the "super column map" internally. Further, due to
what's above, said loop appears to return a definition whose type is the
{{MapType}}, while the code following it clearly return a definition whose type
is the values of said {{MapType}}, so it feels wrong the method would return
different kind of types depending on the path taken. I'm most likely missing
something since your tests seem to be working, but I'm not sure what.
** In {{processPartition}}:
*** There is a typo in a comment, where in "... can't be change to support
inclusive slice ...", I believe you want "exclusive".
*** In the {{REGULAR}} branch of the {{switch}}:
**** in the 2nd branch ({{else if ...}}), we should use {{result.add(cell)}}
directly. Not only does that already handle counters, but more importantly, if
we don't do that, things like {{ttl(v)}} will not work on the compact column
value.
**** the last {{else}} is confusing: by definition we shouldn't be exposing any
other column than the "compact value" column, so that branch should probably
not be there (or be an assertion that we shouldn't get there).
** In the {{updates}} loop in {{prepareUpdateOperations}}, not sure why the 2
first cases seem to assume that {{cfm.isSuperColumnValueColumn(def)}}, but the
3rd case tests it explicitely (but after having assigned {{superColumnValue}}).
In general, not sure why the 3rd case looks different from the other 2. Also,
I find it a bit hard to follow the mix of {{if}} using {{continue}} and
sometime using {{else}}, sometimes not. I'm personally prefer using {{else}}
when needed and remove all the {{continue}}. Lastly, seems the method could
use a check for {{superColumnKey != null}}.
** In {{prepareInsertForJSONOperations}}, I believe the 2nd call to
{{getRawTermForColumn}} is unecessary, you can use the {{raw}} for before the
{{if}}. Further, the overall code of this method feels _very_ similar to the
one in {{prepareInsertOperations}}: can we extract the bulk of what's common?
(the loop seems to be operating on a {{ColumnDefinition}} and the associated
value, so either pass an Iterable<Pair<...>>, or build 2 lists; I don't think
we care about the performance difference here).
** Nit: {{getSuperColumnKeyRelation}} can be private.
** In {{prepareDeleteOperations}}, need to fix the "Maybe not single only"
comment. Either {{IN}} weren't suppored for this in 2.x and it's enough to make
sure it's still refused, or we should support it (note: maybe this is already
tested and is something that wan't allowed in 2.x anyway; if so, let's updated
or remove the comment).
** Code style and minor nits:
*** the 2 lines doing {{if (v instanceof AbstractMarker.Raw)
boundNames.add(...);}} are done enough time that a simple helper might
streamline this.
*** In {{rebuildLWTConditions}}, I'd invert the initial {{if}} check (making
it, imo, easier to read) with a direct return and "de-indent" the bulk of the
method. Also, I'd remove the {{columnConditions.clear()}}: doesn't feel useful,
and feels weird to do it here (what if we make that collection immutable in
future?).
*** In {{isSuperColumnMapColumn}}, can replace the first condition by
{{ColumnDefinition#isRegular}} (not something due to this patch, but a good
occasion to simplify).
*** for 'if-then-else', we never use brakets for one of the branch and not
another (meaning that we only skip brackets if all of the branches are
one-liner). There is violations to that rule in at least {{processPartition}},
{{getColumnFilter}}, {{prepareUpdateOperations}} and
{{prepareDeleteOperations}}.
* In {{SelectStatement}}, I believe that if you move the
{{SuperColumnCompatibility#processPartition}} call at the beginning of
{{SelectStatement#processPartition}}, you'd be able to remove the specific call
in {{ModificationStatement#buildCasFailureResultSet}} (and generally reduce the
changes of misuses).
* Slightly bummed by the addition of the SC special fields in
{{StatementRestrictions}}. Can't we move most of this in
{{SuperColumnCompatibility#getColumnFilter}}, by extracting things there (tbc,
I'm genuninely not sure how easy change that is, and if it's too convoluted,
I'm fine leaving things as they are; I just prefer having a much special casing
in {{SuperColumnCompatibility}} as possible. I also assume we can't entirely
remove all special casing from {{StatementRestrictions}} since we will at least
make sure it doesn't complain about requiring {{ALLOW FILTERING}} if we leave
the "superColumnKeyColumn" restriciton in)?
* In {{ModificationStatement}} why not directly use {{cfm.superCfValueColumn}}
instead of looping over {{cfm.allColumnsInSeelectOrder()}} to find it?
> 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]
