hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Francke" <...@lars-francke.de>
Subject Re: Review Request 23799: 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
Date Thu, 31 Jul 2014 10:45:21 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message