[
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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
