You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Vihang Karajgaonkar <vi...@cloudera.com> on 2016/07/11 20:28:23 UTC
Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/
-----------------------------------------------------------
Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
Repository: hive-git
Description
-------
HIVE-14135 : beeline output not formatted correctly for large column widths
Diffs
-----
beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
Diff: https://reviews.apache.org/r/49919/diff/
Testing
-------
Thanks,
Vihang Karajgaonkar
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/#review142287
-----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BufferedRows.java (line 76)
<https://reviews.apache.org/r/49919/#comment207854>
wondering if this logic can be folded into the prior loop itself ?
max((max[j], row.sizes[j] + 1)),maxColumnWidth) ?
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (line 1)
<https://reviews.apache.org/r/49919/#comment207855>
Missing Apache header
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (line 16)
<https://reviews.apache.org/r/49919/#comment207856>
nit: extra line
- Mohit Sabharwal
On July 12, 2016, 5:51 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -----------------------------------------------------------
>
> (Updated July 12, 2016, 5:51 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-14135 : beeline output not formatted correctly for large column widths
>
>
> Diffs
> -----
>
> beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
> beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
> beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/49919/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/#review142322
-----------------------------------------------------------
Ship it!
Ship It!
- Mohit Sabharwal
On July 14, 2016, 11:12 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -----------------------------------------------------------
>
> (Updated July 14, 2016, 11:12 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-14135 : beeline output not formatted correctly for large column widths
>
>
> Diffs
> -----
>
> beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
> beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
> beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/49919/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/
-----------------------------------------------------------
(Updated July 14, 2016, 11:12 p.m.)
Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
Changes
-------
Added Mohit's suggestions.
Repository: hive-git
Description
-------
HIVE-14135 : beeline output not formatted correctly for large column widths
Diffs (updated)
-----
beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
Diff: https://reviews.apache.org/r/49919/diff/
Testing
-------
Thanks,
Vihang Karajgaonkar
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On July 14, 2016, 9:57 p.m., Vihang Karajgaonkar wrote:
> > beeline/src/java/org/apache/hive/beeline/BufferedRows.java, line 76
> > <https://reviews.apache.org/r/49919/diff/2/?file=1442484#file1442484line76>
> >
> > Thanks for reviewing. I thought of adding it in the above loop like you mentioned, but then went against that for readability reasons since max.length should not be too big (it is numbers of columns in a row). I will add it now in the loop.
> >
> > btw, If we have to add it in the loop it should be
> > Math.min(Math.max(max[j], row.sizes[j] + 1), maxColumnWidth); Did you mean the same?
Ah, right, that should me min not max.
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/#review142292
-----------------------------------------------------------
On July 12, 2016, 5:51 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -----------------------------------------------------------
>
> (Updated July 12, 2016, 5:51 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-14135 : beeline output not formatted correctly for large column widths
>
>
> Diffs
> -----
>
> beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
> beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
> beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/49919/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/#review142292
-----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BufferedRows.java (line 76)
<https://reviews.apache.org/r/49919/#comment207860>
Thanks for reviewing. I thought of adding it in the above loop like you mentioned, but then went against that for readability reasons since max.length should not be too big (it is numbers of columns in a row). I will add it now in the loop.
btw, If we have to add it in the loop it should be
Math.min(Math.max(max[j], row.sizes[j] + 1), maxColumnWidth); Did you mean the same?
- Vihang Karajgaonkar
On July 12, 2016, 5:51 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -----------------------------------------------------------
>
> (Updated July 12, 2016, 5:51 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-14135 : beeline output not formatted correctly for large column widths
>
>
> Diffs
> -----
>
> beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
> beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
> beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/49919/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/
-----------------------------------------------------------
(Updated July 12, 2016, 5:51 p.m.)
Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
Changes
-------
Added Sahil's suggestions.
Repository: hive-git
Description
-------
HIVE-14135 : beeline output not formatted correctly for large column widths
Diffs (updated)
-----
beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
Diff: https://reviews.apache.org/r/49919/diff/
Testing
-------
Thanks,
Vihang Karajgaonkar
Re: Review Request 49919: HIVE-14135 : beeline output not formatted
correctly for large column widths
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49919/#review141920
-----------------------------------------------------------
I took a look at the unit test and the use of Mockito, the approach there looks good, just two minor comments.
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (lines 37 - 39)
<https://reviews.apache.org/r/49919/#comment207347>
You can delete this, right? Since it looks like the method is empty.
beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (line 42)
<https://reviews.apache.org/r/49919/#comment207351>
Could you add a more descriptive name to this method? Also some comments about the test is trying to test would be helpful
- Sahil Takiar
On July 11, 2016, 8:28 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -----------------------------------------------------------
>
> (Updated July 11, 2016, 8:28 p.m.)
>
>
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and Thejas Nair.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-14135 : beeline output not formatted correctly for large column widths
>
>
> Diffs
> -----
>
> beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c
> beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 5aaa38527734d46de037352ff51e54e0ae1cede0
> beeline/src/java/org/apache/hive/beeline/BufferedRows.java 962c5319bb7e6e448979e1cef80a086cadd2ecc6
> beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/49919/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>