----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25468/#review52688 -----------------------------------------------------------
Looks good apart from minor comments. Maybe add a test for the Serialization part? https://issues.apache.org/jira/browse/HIVE-5976 integration might be nice: "STORED AS CSV". Unfortunately there's no documentation yet so I'm not sure if it's feasible. serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91646> This comment doesn't add value so I suggest removing it. serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91647> * Constants is deprecated. Use serdeConstants instead * Exceeds maximum line length (100 chars) serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91648> Unused serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91651> Missing spaces around operators serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91650> 2 x Unnecessary "this" serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91653> Missing spaces around operators serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91669> I suggest moving these properties to Constants somewhere serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91649> Method declared final in final class serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91657> long line serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91656> Missing spaces serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91659> I don't quite get this comment. Looking at the two CSVReader constructors they seem to do the same in this case. From how I understand it this if-statement is not needed. Same for the newWriter method. Maybe I'm missing something? serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java <https://reviews.apache.org/r/25468/#comment91660> Missing @Override annotation serde/src/test/org/apache/hadoop/hive/serde2/TestCSVSerde.java <https://reviews.apache.org/r/25468/#comment91661> Can be private too serde/src/test/org/apache/hadoop/hive/serde2/TestCSVSerde.java <https://reviews.apache.org/r/25468/#comment91662> Properties.put should not be used. Use setProperty instead. Also Constants == deprecated - Lars Francke On Sept. 9, 2014, 2:16 a.m., cheng xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25468/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2014, 2:16 a.m.) > > > Review request for hive. > > > Bugs: HIVE-7777 > https://issues.apache.org/jira/browse/HIVE-7777 > > > Repository: hive-git > > > Description > ------- > > HIVE-7777: add CSVSerde support > > > Diffs > ----- > > serde/pom.xml f8bcc830cfb298d739819db8fbaa2f98f221ccf3 > serde/src/java/org/apache/hadoop/hive/serde2/CSVSerde.java PRE-CREATION > serde/src/test/org/apache/hadoop/hive/serde2/TestCSVSerde.java PRE-CREATION > > Diff: https://reviews.apache.org/r/25468/diff/ > > > Testing > ------- > > Unit test > > > Thanks, > > cheng xu > >
