[jira] [Commented] (CASSANDRA-10857) Allow dropping COMPACT STORAGE flag from tables in 3.X

["Sylvain Lebresne (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-10857?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16209348#comment-16209348 ] 

Sylvain Lebresne commented on CASSANDRA-10857:
----------------------------------------------

I definitively like the approach of the patch, and it looks largely good.

As we discussed offline, we should test 2ndary indexes. Other than that, I have a few smaller remarks/nits that I'm going to list below, but I took the liberty to push 3 commits [here|https://github.com/pcmanus/cassandra/commits/10857-review] fixing those to move faster. Feel free to push back on some of it, but here's what those commit do:
* The first commit has various nits, including some around making language a bit more consistent. It also toy around the {{validateColumnFamily}} a bit: I figured that since we were calling it most of the time with the thrid parameter being {{false}}, we could have an override without that third argument. Then it occured to me that moving those methods back to {{ThriftValidation}} would remove quite a bit from the diff of this ticket, which made it easier to review so I did that. If you care about those methods being in {{Schema}}, I can move them back again, and I agree on principle that {{Schema}} is a better home for them, but it's all fixed in trunk and 3.x branches are in maintenance mode, so felt like minimizing code moved around was a decent idea; ¯\_(ツ)_/¯.
* In the parser, I was a bit worried about allowing empty strings in {{QUOTED_NAME}} directly, because that is used in very many places and it felt hard/error prone to check all impacted places (and I think some places were missing in the patch; typically, I don't think we should let user add column with empty names but it doesn't seem ALTER TABLE was preventing that). I suggest lowering that risk by limiting the places where we do allow this directly in the parser.  Thanksfully, that's reasonably easy because the parser already had the {{cident}}, which is used in place where we could get a thrift-created column name (as those required a special handling), and it's exactly the same places we want to support here. This is the 2nd commit.
* The patch mentions in a number of places that schema changes were not allowed in the no-compact mode, which sounds fine, but I didn't find the place where that was enforced.  So I added it in {{SchemaAlteringStatement}} directly (and I've let changes on keyspaces/non compact tables go because no real reason to refuse them).  That's the third commit.  

The last (but not least) remark is around the {{NEWS.txt}} changes. First, as it's in the section for 3.0.15, I'm not sure having details there is the best place. Second, some of it will be confusing to users because things like "sparse/dense" and "compact value columns" are stuffs we use internally but aren't something users know about. We can rephrase to make it user-understandable, but that will expand the whole section, and back to the first point: not the right place for that.

Tl;dr, what I think we should do is shrink that {{NEWS.txt}} section (just say we've added DROP COMPACT STORAGE and a new mode to make transitions easier) and link to a proper documentation section (to be written).  I'll note that the new doc isn't in 3.0 so I the new section will have to be added to a 3.11 patch, but it doesn't feel like a big deal to have the 3.0 news file point to the 3.11 documentation.


> Allow dropping COMPACT STORAGE flag from tables in 3.X
> ------------------------------------------------------
>
>                 Key: CASSANDRA-10857
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-10857
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: CQL, Distributed Metadata
>            Reporter: Aleksey Yeschenko
>            Assignee: Alex Petrov
>            Priority: Blocker
>             Fix For: 3.0.x, 3.11.x
>
>
> Thrift allows users to define flexible mixed column families - where certain columns would have explicitly pre-defined names, potentially non-default validation types, and be indexed.
> Example:
> {code}
> create column family foo
>     and default_validation_class = UTF8Type
>     and column_metadata = [
>         {column_name: bar, validation_class: Int32Type, index_type: KEYS},
>         {column_name: baz, validation_class: UUIDType, index_type: KEYS}
>     ];
> {code}
> Columns named {{bar}} and {{baz}} will be validated as {{Int32Type}} and {{UUIDType}}, respectively, and be indexed. Columns with any other name will be validated by {{UTF8Type}} and will not be indexed.
> With CASSANDRA-8099, {{bar}} and {{baz}} would be mapped to static columns internally. However, being {{WITH COMPACT STORAGE}}, the table will only expose {{bar}} and {{baz}} columns. Accessing any dynamic columns (any column not named {{bar}} and {{baz}}) right now requires going through Thrift.
> This is blocking Thrift -> CQL migration for users who have mixed dynamic/static column families. That said, it *shouldn't* be hard to allow users to drop the {{compact}} flag to expose the table as it is internally now, and be able to access all columns.



--
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