Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Wed, 6 Apr 2016 03:06:25 +0000 (UTC)
From: "Stefania (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12909152.1446197496000.146458.1459911985621@Atlassian.JIRA>
In-Reply-To: <JIRA.12909152.1446197496000@Atlassian.JIRA>
References: <JIRA.12909152.1446197496000@Atlassian.JIRA>
 <JIRA.12909152.1446197496114@arcas>
Subject: [jira] [Commented] (CASSANDRA-10624) Support UDT in
 CQLSSTableWriter
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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