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

Stefania commented on CASSANDRA-10624:
--------------------------------------

This patch is really good and I am really impressed with how quickly you 
created it.

I have only one major suggestion, which is to get rid of the codec map and the 
list of user types in the builder, and instead use Types in the builder and 
then add them to the keyspace metadata. This has also the advantage that 
duplicate type names are detected when calling withType. It's easier to show, 
so I created a commit with some suggestions 
[here|https://github.com/stef1927/cassandra/commit/afe170ad07045e75ad0f7c96e4dcedc03ca3230b].

*Nits*
* In CQLSStableWriter.getTableMetadata() at line 562 the comment is no longer 
relevant.
* In this same method, it would be nice to have an IllegalArgumentException 
check, just like you did for withType.
* In some locations the curly brackets are not positioned on a new line, I 
spotted them in CQLSStableWriter at lines 295, 609 and in CQLSStableWriterTest 
at line 359.

I think only the second point was not done in the commit with suggestions 
above, but double check on the brackets.

*Continuous integration*
I've launched some jobs against my GH branch, they are still running:

http://cassci.datastax.com/job/stef1927-10624-testall
http://cassci.datastax.com/job/stef1927-10624-dtest

You can ping "exlt" or "ptnapoleon" in IRC to set-up your GH account to run CI 
on cassci.datastax.com.

> Support UDT in CQLSSTableWriter
> -------------------------------
>
>                 Key: CASSANDRA-10624
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-10624
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: CQL
>            Reporter: Sylvain Lebresne
>            Assignee: Alex Petrov
>             Fix For: 3.x
>
>         Attachments: 0001-Add-support-for-UDTs-to-CQLSStableWriter.patch
>
>
> As far as I can tell, there is not way to use a UDT with {{CQLSSTableWriter}} 
> since there is no way to declare it and thus {{CQLSSTableWriter.Builder}} 
> knows of no UDT when parsing the {{CREATE TABLE}} statement passed.
> In terms of API, I think the simplest would be to allow to pass types to the 
> builder in the same way we pass the table definition. So something like:
> {noformat}
> String type = "CREATE TYPE myKs.vertex (x int, y int, z int)";
> String schema = "CREATE TABLE myKs.myTable ("
>               + "  k int PRIMARY KEY,"
>               + "  s set<vertex>"
>               + ")";
> String insert = ...;
> CQLSSTableWriter writer = CQLSSTableWriter.builder()
>                                           .inDirectory("path/to/directory")
>                                           .withType(type)
>                                           .forTable(schema)
>                                           .using(insert).build();
> {noformat}
> I'll note that implementation wise, this might be a bit simpler after the 
> changes of CASSANDRA-10365 (as it makes it easy to passe specific types 
> during the preparation of the create statement).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to