----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27918/#review61043 -----------------------------------------------------------
Overall it looks good. Just couple of small nits: common/src/main/java/org/apache/sqoop/schema/type/AbstractComplexType.java <https://reviews.apache.org/r/27918/#comment102505> Could you talk a bit about what does the size represents here? (and prehaps include a comment describing that) common/src/main/java/org/apache/sqoop/schema/type/Map.java <https://reviews.apache.org/r/27918/#comment102503> I'm assuming that Value also should not be null, correct? Jarcec - Jarek Cecho On Nov. 12, 2014, 7:45 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27918/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2014, 7:45 p.m.) > > > Review request for Sqoop and Jarek Cecho. > > > Bugs: SQOOP-1709 > https://issues.apache.org/jira/browse/SQOOP-1709 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > NOTE: WS will be addressed once the patch is reviewd for functinality > > Have a few questions as part of this patch: > > Why do we make the name optional? It actually throws an exception if we have > more than one column as empty. So would it not be wise to make it a > mandatory? or can we have columns with no names in databases? > > Second, the length attribute, is it more like maxLength that the columns > have? if so we should just call it so. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java > ecb5aa3 > > common/src/main/java/org/apache/sqoop/schema/type/AbstractComplexListType.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/schema/type/AbstractComplexType.java > ca9b3b1 > common/src/main/java/org/apache/sqoop/schema/type/AbstractDateTime.java > 5c969af > common/src/main/java/org/apache/sqoop/schema/type/AbstractNumber.java > 1ecb2d5 > > common/src/main/java/org/apache/sqoop/schema/type/AbstractPrimitiveType.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/schema/type/AbstractString.java > 950c2fd > common/src/main/java/org/apache/sqoop/schema/type/Array.java d7dcc0c > common/src/main/java/org/apache/sqoop/schema/type/Binary.java 7111bf5 > common/src/main/java/org/apache/sqoop/schema/type/Bit.java 2371746 > common/src/main/java/org/apache/sqoop/schema/type/Column.java 2339d66 > common/src/main/java/org/apache/sqoop/schema/type/Date.java 324a089 > common/src/main/java/org/apache/sqoop/schema/type/DateTime.java d982d2a > common/src/main/java/org/apache/sqoop/schema/type/Decimal.java 5470b9c > common/src/main/java/org/apache/sqoop/schema/type/Enum.java 0c54fd0 > common/src/main/java/org/apache/sqoop/schema/type/FixedPoint.java d949aa7 > common/src/main/java/org/apache/sqoop/schema/type/FloatingPoint.java > 9639197 > common/src/main/java/org/apache/sqoop/schema/type/Map.java d3301c0 > common/src/main/java/org/apache/sqoop/schema/type/Set.java a18699f > common/src/main/java/org/apache/sqoop/schema/type/Text.java db47ede > common/src/main/java/org/apache/sqoop/schema/type/Time.java 0f71508 > common/src/main/java/org/apache/sqoop/schema/type/Unknown.java 97f72c0 > > common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java > 1640ead > > Diff: https://reviews.apache.org/r/27918/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
