You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by pengcheng xiong <px...@hortonworks.com> on 2015/04/03 08:24:30 UTC
Review Request 32809: Disallow create table with dot/colon in column
name
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/
-----------------------------------------------------------
Review request for hive, Ashutosh Chauhan and John Pullokkaran.
Repository: hive-git
Description
-------
Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
Diff: https://reviews.apache.org/r/32809/diff/
Testing
-------
Thanks,
pengcheng xiong
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by Swarnim Kulkarni <ku...@gmail.com>.
> On April 7, 2015, 4:45 a.m., Swarnim Kulkarni wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g, line 635
> > <https://reviews.apache.org/r/32809/diff/1/?file=914560#file914560line635>
> >
> > This and the following line can be simplified as
> >
> > return input.indexOf(c);
>
> pengcheng xiong wrote:
> The purpose of the function is to test whether a string contains a char. The actual index is only used to check if it is there, the detailed position information is not needed. That is to say, a boolean return value is enough for my purpose.
Yup. I think your logic is correct. Just having a return input.indexOf(c); serves the same purpose but is simpler. :)
- Swarnim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/#review79121
-----------------------------------------------------------
On April 7, 2015, 6:18 p.m., pengcheng xiong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32809/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 6:18 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and John Pullokkaran.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
> ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32809/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengcheng xiong
>
>
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by pengcheng xiong <px...@hortonworks.com>.
> On April 7, 2015, 4:45 a.m., Swarnim Kulkarni wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g, line 632
> > <https://reviews.apache.org/r/32809/diff/1/?file=914560#file914560line632>
> >
> > If you choose to use a String.contains, this could as well be a character array.
Thanks for your comment. But I assume that char is enough for my purpose.
> On April 7, 2015, 4:45 a.m., Swarnim Kulkarni wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g, line 634
> > <https://reviews.apache.org/r/32809/diff/1/?file=914560#file914560line634>
> >
> > Why not simply use string.contains here?
The contains method is implemented using a call to indexOf, so they are essentially the same.
public boolean contains(CharSequence s) {
return indexOf(s.toString()) > -1;
}
But in my case, I just would like to check if a string contains a char, rather than a CharSequence. Thus, I think indexOf would be better
> On April 7, 2015, 4:45 a.m., Swarnim Kulkarni wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g, line 635
> > <https://reviews.apache.org/r/32809/diff/1/?file=914560#file914560line635>
> >
> > This and the following line can be simplified as
> >
> > return input.indexOf(c);
The purpose of the function is to test whether a string contains a char. The actual index is only used to check if it is there, the detailed position information is not needed. That is to say, a boolean return value is enough for my purpose.
- pengcheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/#review79121
-----------------------------------------------------------
On April 3, 2015, 6:24 a.m., pengcheng xiong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32809/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 6:24 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and John Pullokkaran.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
> ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32809/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengcheng xiong
>
>
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by Swarnim Kulkarni <ku...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/#review79121
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
<https://reviews.apache.org/r/32809/#comment128237>
If you choose to use a String.contains, this could as well be a character array.
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
<https://reviews.apache.org/r/32809/#comment128236>
Why not simply use string.contains here?
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
<https://reviews.apache.org/r/32809/#comment128234>
This and the following line can be simplified as
return input.indexOf(c);
ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java
<https://reviews.apache.org/r/32809/#comment128235>
Nit: Complete the comment.
- Swarnim Kulkarni
On April 3, 2015, 6:24 a.m., pengcheng xiong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32809/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 6:24 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and John Pullokkaran.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
> ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32809/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengcheng xiong
>
>
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by John Pullokkaran <jp...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/#review79229
-----------------------------------------------------------
Ship it!
Ship It!
- John Pullokkaran
On April 7, 2015, 6:18 p.m., pengcheng xiong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32809/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 6:18 p.m.)
>
>
> Review request for hive, Ashutosh Chauhan and John Pullokkaran.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
> ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32809/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengcheng xiong
>
>
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by pengcheng xiong <px...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/
-----------------------------------------------------------
(Updated April 7, 2015, 6:18 p.m.)
Review request for hive, Ashutosh Chauhan and John Pullokkaran.
Changes
-------
thanks for Swarnim Kulkarni's comments. I tried to answer and address them.
Repository: hive-git
Description
-------
Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
Diff: https://reviews.apache.org/r/32809/diff/
Testing
-------
Thanks,
pengcheng xiong
Re: Review Request 32809: Disallow create table with dot/colon in
column name
Posted by John Pullokkaran <jp...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32809/#review79203
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
<https://reviews.apache.org/r/32809/#comment128410>
Please address Swarnism's comments
- John Pullokkaran
On April 3, 2015, 6:24 a.m., pengcheng xiong wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32809/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 6:24 a.m.)
>
>
> Review request for hive, Ashutosh Chauhan and John Pullokkaran.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Since we don't allow users to query column names with dot in the middle such as emp.no, don't allow users to create tables with such columns that cannot be queried. Fix the documentation to reflect this fix.
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 2e583da
> ql/src/test/org/apache/hadoop/hive/ql/parse/TestUnpermittedCharsInColumnNameCreateTableNegative.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32809/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> pengcheng xiong
>
>