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 Wed, 30 Jul 2014 10:51:31 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23799/#review49091
-----------------------------------------------------------


In general this feels a bit awkward. I think better CSV/TSV support is a good idea but "quotedCsv"
seems misleading as the old "csv" and "tsv" now quote as well if the separator is contained
in the column value.


beeline/src/java/org/apache/hive/beeline/BeeLine.java
<https://reviews.apache.org/r/23799/#comment85924>

    Missing space here and next line



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85920>

    remove "this" and call to "getSeparator", can just be "separator".



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85915>

    Can be converted to a variable arity function (e.g. String... vals)



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85916>

    Rename to writer?



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85917>

    Same as above: Can be converted to variable arity method



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85918>

    ...variable arity



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85919>

    Remove this and probably replace the call to isSingleQuoted with just "singleQuoted",
no need to go through a simple getter



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85923>

    Missing spaces around the else



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85922>

    I'd either remove the getter and setters entirely or they need changing so that things
are properly updated when separator/singleQuoted/csvPreference are changed.
    
    Example: Someone passes in a CsvPreference with a different separator than the one set
in here.
    
    I think part of this patch needs to be the removal of all these simple (getter/)setters.
    
    If you don't want that then you need some verification logic that things make sense.



beeline/src/java/org/apache/hive/beeline/SeparatedValuesOutputFormat.java
<https://reviews.apache.org/r/23799/#comment85921>

    This is not a getter but a setter.


- Lars Francke


On July 30, 2014, 8:30 a.m., cheng xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23799/
> -----------------------------------------------------------
> 
> (Updated July 30, 2014, 8:30 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/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