[
https://issues.apache.org/jira/browse/CASSANDRA-7859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14185402#comment-14185402
]
Sylvain Lebresne commented on CASSANDRA-7859:
---------------------------------------------
I'm not totally convinced about adding new Frozen*Type classes. It adds a bunch
of code duplication and changes (and breaks a number of instanceof checks, see
my last remark below) for unclear benefis imo. Instead, I'd prefer adding a
{{isFrozen}} (or {{isMultiCell}}) in CollectionType. This would also remove
also the {{IMap}}, {{ISet}}, {{IList}} and {{MultiCollectionCellType}} and
would save a ton of diff from the patch.
Also I think those new classes adds some confusion (and somewhat break backward
compatibility) in the sense that if you have
{noformat}
CREATE TYPE myType (s set<int>)
CREATE TABLE foo (k frozen<myType> PRIMARY KEY)
{noformat}
then for table create before this patch, the validator for {{k}} will be
{{UserType(foo, s:SetType(Int32Type))}}, while after this patch it will be
{{UserType(foo, s:FrozenSetType(Int32Type))}}.
So I think we'd also need to be a tad smarter when dealing with
serializing/deserializing types: we'll continue to serialize the type below as
{{UserType(foo, s:SetType(Int32Type))}} for compatibility sake, but when
parsing that, we'd remember the set is inside a UDT and thus
frozen/non-multi-cell. When we have frozen collections, say
{{frozen<set<list<int>>>}}, we could serialize it as
{{FrozenType(SetType(ListType(Int32Type)))}}, which would be easier to
handlefor drivers (when they read/decode schema tables).
Other remarks:
* The commit seems to include changes from CASSANDRA-6904, and some changes in
StorageService/StreamCoordinator that are unrelated to this ticket.
* I wonder if we shouldn't allow {{frozen}} on any type syntax wise. This would
simply have no impact on simple types but that's already what happens for
tuples after this patch so this would be somewhat consistent. I suspect this
might simplify validation at least.
* In {{CQL3Type.Raw.RawCollection}}, in the {{prepare}} method, we could
replace {{if (!frozen && values.isCollection() && !values.frozen)}} by {{if
(!frozen && values.supportsFreezing() && !values.frozen)}} so it works properly
once we have non-frozen UDT. Also, I think the {{keys}} and {{values}} field
can stay {{final}}.
* A bit sad you didn't found a more brilliant name than my {{fullCollection}}.
And I wonder if we shouldn't make it a bit more generic so that if we want to
index full (frozen) UDT in the future we don't have to introduce a {{fullUDT}}
and so on. What about generalizing to {{wholeValue}} (which is still a crappy
name so better naming ideas welcome)?
* In {{Tuples}} and {{UserTypes}} {{bindInternal}} methods, not sure using
{{isMultiCell}} over the existing {{isCollection}} is a good idea.
* In {{SelectStatement.getRequestedColumns}}, the changes don't seem related to
this issue, are they?
* In {{SelectStatement.buildBound}}, the {{!r.isContains()}} test is a no-op
since the first {{isNullRestriction()}} branch will have been taken. Also not
convinced having the {{CONTAINS} test in {{isNullRestriction}} is very
intuitive. I'd rather replace the test by {{if (isNullRestriction(r, b) ||
!r.canEvaluateWithSlice())}} directly in {{buildBound}}.
* In {{SingleColumnRestriction.Contains.canEvaluateWithSlices}}, I don't
understand the comment.
* Why do we need the newly added code in {{SecondaryIndexSearcher.validate}}?
* In {{CompositesIndexOnCollectionKey.supportsOperator}}, let's add parenthesis
rather than relying on the precedence of {{&&}} over {{||}}.
* Note 100% sure about the {{isValueCompatible()}} for frozen collections.
Frozen collection values are stored sorted, and so it feels allowing changes
that modify the sorting could backfire in subtle ways. I haven't carefully
checked if it would actually break or not, but I'd prefer keeping it more
restrictive initially, and if many people complain about the restriction, we
can think long and hard about whether that's safe or not.
* As the patch stand, there is a few {{instanceof MapType}}, {{instanceof
ListType}} and {{instanceof SetType}} that should have a {{I}} added in front
(but as said above, I'd rather just get rid of those new classes entirely).
> Extend freezing to collections
> ------------------------------
>
> Key: CASSANDRA-7859
> URL: https://issues.apache.org/jira/browse/CASSANDRA-7859
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Tyler Hobbs
> Labels: cql
> Fix For: 2.1.2
>
> Attachments: 7859-v1.txt
>
>
> This is the follow-up to CASSANDRA-7857, to extend {{frozen}} to collections.
> This will allow things like {{map<text, frozen<map<int, int>>>}} for
> instance, as well as allowing {{frozen}} collections in PK columns.
> Additionally (and that's alsmot a separate ticket but I figured we can start
> discussing it here), we could decide that tuple is a frozen type by default.
> This means that we would allow {{tuple<int, text>}} without needing to add
> {{frozen}}, but we would require {{frozen}} for complex type inside tuple, so
> {{tuple<int, list<text>>}} would be rejected, but not {{tuple<int,
> frozen<list<text>>>}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
