[
https://issues.apache.org/jira/browse/CASSANDRA-13426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287636#comment-16287636
]
Sam Tunnicliffe commented on CASSANDRA-13426:
---------------------------------------------
I've been looking at the WIP branch for a little while now. Looks good, lots of
great improvements. I have a few, mostly minor comments:
* AlterSchemaStatement
** In grantPermissionsOnResource can use AuthenticatedUser::getPrimaryRole
* QueryProcessor
** #402 assert is redundant
** #422 typo "MDnt5"
** Should Prepared belong to QueryHandler rather than QP?
* ClientState
** ensureIsSuperUser is the only place we camel case SuperUser
* TableMetadata/TableParams/Views
** Maybe a bit subjective, but I find the naming of "unbuild" unintuitive. How
about something like builderFrom/makeBuilder/asBuilder/toBuilder ?
* Views
** metadatas() method should be renamed - data is already plural, but also
could it be named to reflect the fact that it returns an
Iterable<TableMetadata>, whilst Views itself implements Iterable<ViewMetadata>.
It's a bit ambiguous as it is.
* TableId
** Typo in javadoc: s/nicely name class make/nicely named class makes
* ViewMetadata
** equals uses WhereClause::equals, but this isn't overridden
** the comment in withRenamedPrimaryKeyColumns belongs in
WhereClause::renameIdentifier, if it's actually necessary
* TableMetadata
** toDebugString is unused
* Tables
** getNullable is missing annotation (all other similar methods have it).
* Keyspaces
** get(String name) is unused
* KeyspaceDiff
** A comment explaining why function diffs need to be handled differently could
be useful (I'm assuming it's because the filtering to distinguish UDFs/UDAs
makes it slightly more expensive than the other types of diff).
* Functions
** can Filter.test use isAggregate rather than instanceof ?
** aggregatesUsingFunction, comment should reads s/collection/stream
* CompressionParams
** outstanding TODO on setCrcCheckChance
* SetType/ListType/MapType/DynamicCompositeType
** getInstance can be simplified to just return the result of computeIfAbsent.
There's an unchecked warning, but I'm not sure that's any different from the
existing one. ReversedType and CompositeType impls already do this.
* AbstractType
** I think the comment on withUpdatedUserType could be a little clearer. Maybe
something like "Returns an instance with all references to the supplied
UserTypei recursively replaced with its new definition".
* SelectStatement
** The order of constructor args was changed to swap TableMetadata & the bind
variables. NBD, but having the TM first seems more logical to me, was there a
reason behind the switch?
* QualifiedName
** Typo in class level javadoc - s/CLass/Class
* StatementRestrictions
** The comment on the new ctor is slightly incorrect. We want to override the
allow 2i flag from the StatementType for MV statements to avoid initing the the
KS and SecondaryIndexManager. We don't open the KS to determine the value of
the allow flag in either case.
* UDAggregate
** When reconstructing from schema tables and the function can't be
reconstructed for whatever reason - we preserve the old behaviour with a dummy,
broken function for UDFs but not for UDAs. These now trigger an assert. Is this
an issue?
* DynamicCompositeType
** needs an expandUserTypes impl?
** withUpdatedUserType won't work correctly now that referencesUserType is
inherited from AbstractType
* UserType
** expandUserType override isn't necessary, though I'm OK with leaving it in
for clarity
* MigrationManager
** Comment on forceAnnounceNewTable -
s/announceUpdateColumnFamily/announceTableUpdate
* CreateViewStatement
** validation is now stricter in that it doesn't silently accept SELECT that
doesn't contain all primary key elements, or PK columns without IS NOT NULL.
It's good, just noting that this should be documented somewhere.
* UFTest
** line #114 seems to have a typo - s/string1/string
* VariableSpecifications/CQLStatement
** there's a minor inconsistency in method naming here:
getPartitionKeyBindIndexes vs getPartitionKeyBindVariableIndexes.
> Make all DDL statements idempotent and not dependent on global state
> --------------------------------------------------------------------
>
> Key: CASSANDRA-13426
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13426
> Project: Cassandra
> Issue Type: Sub-task
> Components: Distributed Metadata
> Reporter: Aleksey Yeschenko
> Assignee: Aleksey Yeschenko
> Fix For: 4.0
>
>
> A follow-up to CASSANDRA-9425 and a pre-requisite for CASSANDRA-10699.
> It's necessary for the latter to be able to apply any DDL statement several
> times without side-effects. As part of the ticket I think we should also
> clean up validation logic and our error texts. One example is varying
> treatment of missing keyspace for DROP TABLE/INDEX/etc. statements with IF
> EXISTS.
--
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
