----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23799/#review49214 -----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BeeLine.java <https://reviews.apache.org/r/23799/#comment86103> Probably a good idea to move the "|" to a public static final in BeelineOpts beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java <https://reviews.apache.org/r/23799/#comment86102> Now, I have no idea how that works myself but it'd be fabulous if we could specify \001 etc. here as well. It is Hive's default separator after all. Not sure how that parsing stuff is handled so take this as an optional idea but by no means necessary. beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java <https://reviews.apache.org/r/23799/#comment86096> I think this comment can go now :) beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java <https://reviews.apache.org/r/23799/#comment86093> 1) I know it was public before but a public constructor in a package level class doesn't make sense so it can be removed. 2) Line length in Hive is 100 chars so this can be on one line beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java <https://reviews.apache.org/r/23799/#comment86097> Instead of initializing it here like this you could get rid of the new parameter and check in getFormattedStr if csvPreference is null. If it is then check in the opts if DSV is selected and which delimiter was specified. That way you wouldn't create a new csvPreference for each row either. Either that or pass in the proper delimiter from the start, see my comment in Beeline beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java <https://reviews.apache.org/r/23799/#comment86099> false is the default so no need to specify here beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java <https://reviews.apache.org/r/23799/#comment86100> I think all these setters/getters should go. If not you need to make sure to create a new csvPreference when setting the separator and setting the separator when setting a new csvPreference etc. These methods are not used anywhere (setSeparator is used in the constructor but that can go) anyway. beeline/src/main/resources/BeeLine.properties <https://reviews.apache.org/r/23799/#comment86101> delimiterForCSV -> delimiterForDSV? And probably mention that it's | by default - Lars Francke On July 31, 2014, 5:34 a.m., cheng xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23799/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 5:34 a.m.) > > > Review request for hive. > > > Bugs: HIVE-7390 > https://issues.apache.org/jira/browse/HIVE-7390 > > > Repository: hive-git > > > Description > ------- > > HIVE-7390: refactor csv output format with in RFC mode and add one more > option to support formatting as the csv format in hive cli > > > Diffs > ----- > > beeline/pom.xml 6ec1d1aff3f35c097aa6054aae84faf2d63854f1 > beeline/src/java/org/apache/hive/beeline/BeeLine.java > 528a98e29c23421f9352bdf7c5edd3a9fae0e3ea > beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java > 75f7d38cb97fb753a8f39c19488b9ce0a8d77590 > beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java > 7853c3f38f3c3fb9ae0b9939c714f1dc940ba053 > beeline/src/main/resources/BeeLine.properties > 390d062b8dc52dfa790c7351f3db44c1e0dd7e37 > > itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeeLineWithArgs.java > 8888bd97aff5959fd9040fc0f0a1f6b782f2aa6f > pom.xml b5a5697e6a3b689c2b244ba0338be541261eaa3d > > Diff: https://reviews.apache.org/r/23799/diff/ > > > Testing > ------- > > > Thanks, > > cheng xu > >
