You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Miklos Csanady <mi...@cloudera.com> on 2016/08/11 14:19:56 UTC
Review Request 50982: HIVE-14345:Beeline result table has erroneous
characters
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
Review request for hive, Peter Vary and Sergio Pena.
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
> On Aug. 12, 2016, 4:54 p.m., Vihang Karajgaonkar wrote:
> > beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java, line 100
> > <https://reviews.apache.org/r/50982/diff/1/?file=1470785#file1470785line100>
> >
> > Is it possible to avoid hardcoding this string? If in the future beelineOpts.maxColoumnWidth default values are changed, this test might fail. I think it will be more robust to determine this string programatically using the values in mockResultData and value of maxColumnWidth.
I have changed for shorter strings in text, so it is now independent from maxColumnWidth.
- Miklos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145631
-----------------------------------------------------------
On Aug. 12, 2016, 8:16 p.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2016, 8:16 p.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Bugs: HIVE-14345
> https://issues.apache.org/jira/browse/HIVE-14345
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> Thanks,
>
> Miklos Csanady
>
>
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145631
-----------------------------------------------------------
Fix it, then Ship it!
Overall, the fix looks good. Some comments below.
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java (line 63)
<https://reviews.apache.org/r/50982/#comment211946>
nit, use the formatter to correct the spaces
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (line 35)
<https://reviews.apache.org/r/50982/#comment211945>
I don't think the author comment is required. If you use Eclipse, I found importing the coding-style "eclipse-styles.xml" in dev-support directory helpful to format the code using the coding conventions. This will also remove all the trailing spaces (seen in red below)
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (line 100)
<https://reviews.apache.org/r/50982/#comment211950>
Is it possible to avoid hardcoding this string? If in the future beelineOpts.maxColoumnWidth default values are changed, this test might fail. I think it will be more robust to determine this string programatically using the values in mockResultData and value of maxColumnWidth.
- Vihang Karajgaonkar
On Aug. 12, 2016, 3:16 p.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2016, 3:16 p.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Bugs: HIVE-14345
> https://issues.apache.org/jira/browse/HIVE-14345
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> File Attachments
> ----------------
>
> corrected version
> https://reviews.apache.org/media/uploaded/files/2016/08/12/aed736a0-95f1-4f5b-9bda-4db767911065__HIVE-14345.patch
>
>
> Thanks,
>
> Miklos Csanady
>
>
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145793
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Vary
On Aug. 13, 2016, 5:34 a.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2016, 5:34 a.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Bugs: HIVE-14345
> https://issues.apache.org/jira/browse/HIVE-14345
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> Thanks,
>
> Miklos Csanady
>
>
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145792
-----------------------------------------------------------
Thanks Miklos!
- Peter Vary
On Aug. 13, 2016, 5:34 a.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2016, 5:34 a.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Bugs: HIVE-14345
> https://issues.apache.org/jira/browse/HIVE-14345
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> Thanks,
>
> Miklos Csanady
>
>
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 13, 2016, 5:34 a.m.)
Review request for hive, Peter Vary and Sergio Pena.
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145674
-----------------------------------------------------------
One little nit. Otherwise LGTM
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (lines 50 - 51)
<https://reviews.apache.org/r/50982/#comment212017>
nit: should be one line
- Peter Vary
On Aug. 12, 2016, 8:36 p.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2016, 8:36 p.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Bugs: HIVE-14345
> https://issues.apache.org/jira/browse/HIVE-14345
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> Thanks,
>
> Miklos Csanady
>
>
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 8:36 p.m.)
Review request for hive, Peter Vary and Sergio Pena.
Changes
-------
Removed author tag. Hopefully all others are fixed.
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 8:16 p.m.)
Review request for hive, Peter Vary and Sergio Pena.
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 8:15 p.m.)
Review request for hive, Peter Vary and Sergio Pena.
Changes
-------
New patch file is uploded
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs (updated)
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
File Attachments
----------------
corrected version
https://reviews.apache.org/media/uploaded/files/2016/08/12/aed736a0-95f1-4f5b-9bda-4db767911065__HIVE-14345.patch
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 3:16 p.m.)
Review request for hive, Peter Vary and Sergio Pena.
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
File Attachments (updated)
----------------
corrected version
https://reviews.apache.org/media/uploaded/files/2016/08/12/aed736a0-95f1-4f5b-9bda-4db767911065__HIVE-14345.patch
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Miklos Csanady <mi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 8:45 a.m.)
Review request for hive, Peter Vary and Sergio Pena.
Changes
-------
uploaded new patch file, set branch and bugs label
Bugs: HIVE-14345
https://issues.apache.org/jira/browse/HIVE-14345
Repository: hive-git
Description
-------
Fixed output table formatting header and footer lines.
Diffs
-----
beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
Diff: https://reviews.apache.org/r/50982/diff/
Testing
-------
See attached Unit testClass.
After building with patch, the bug eliminated.
Thanks,
Miklos Csanady
Re: Review Request 50982: HIVE-14345:Beeline result table has
erroneous characters
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50982/#review145509
-----------------------------------------------------------
Hi Miklos,
Thanks for the patch. This extra column was hurting my eyes :)
One important note:
- Please review your patch, to adhere the coding conventions here: https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions
What I have seen specifically:
- Spaces in empty lines
- Padding of lines
- Missing space before '{'
- Missing space between method call parameters
- And in the conditional statement, the extra space made it less readable.
About filling the review request:
- Please fill the branch, and the bug information of this apache review as well, to help searching for it.
Thanks,
Peter
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (lines 71 - 88)
<https://reviews.apache.org/r/50982/#comment211755>
I would remove these empty functions
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (line 91)
<https://reviews.apache.org/r/50982/#comment211754>
nit: Maybe it would be good to have a little more explanation here, what we are testing - like extra column...
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (line 95)
<https://reviews.apache.org/r/50982/#comment211756>
nit: I would remove this line
beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java (line 99)
<https://reviews.apache.org/r/50982/#comment211753>
nit: I think, you should remove this line from the final patch
- Peter Vary
On Aug. 11, 2016, 2:19 p.m., Miklos Csanady wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50982/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 2:19 p.m.)
>
>
> Review request for hive, Peter Vary and Sergio Pena.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Fixed output table formatting header and footer lines.
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/TableOutputFormat.java 2753568
> beeline/src/test/org/apache/hive/beeline/TestTableOutputFormat.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50982/diff/
>
>
> Testing
> -------
>
> See attached Unit testClass.
> After building with patch, the bug eliminated.
>
>
> Thanks,
>
> Miklos Csanady
>
>