hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ashutosh Chauhan" <hashut...@apache.org>
Subject Re: Review Request 16299: HIVE-6013: Supporting Quoted Identifiers in Column Names
Date Wed, 18 Dec 2013 00:59:54 GMT

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



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/16299/#comment58532>

    class PatternValidator was recently introduced in HiveConf, which doesn't let user to
specify invalid value for a config key. Using that here will be useful.



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
<https://reviews.apache.org/r/16299/#comment58545>

    Shall we remove this  if() altogether and thus also above newly introduced method?



ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
<https://reviews.apache.org/r/16299/#comment58546>

    conf should be null here. If it is null, then its a bug. Also, returning null in those
cases seems incorrect. Lets remove this null conf check.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
<https://reviews.apache.org/r/16299/#comment58584>

    Since this method always return true, no need for this if block.



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g
<https://reviews.apache.org/r/16299/#comment58585>

    There can never be the case that hiveconf == null. That will be a bug. Lets remove this
null check.



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g
<https://reviews.apache.org/r/16299/#comment58586>

    It will be good to document where all Identifier is used. Can be lifted straight from
your html document.
    



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g
<https://reviews.apache.org/r/16299/#comment58587>

    Good to add a note here saying QuotedIdentifier only optionally available for columns
as of now.



ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g
<https://reviews.apache.org/r/16299/#comment58588>

    Not related for this patch, but if you feel like it, ll be good to add comment about where
CharSetNames are used. Not necessary though.


- Ashutosh Chauhan


On Dec. 16, 2013, 10:22 p.m., Harish Butani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16299/
> -----------------------------------------------------------
> 
> (Updated Dec. 16, 2013, 10:22 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan and Alan Gates.
> 
> 
> Bugs: HIVE-6013
>     https://issues.apache.org/jira/browse/HIVE-6013
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Hive's current behavior on Quoted Identifiers is different from the normal interpretation.
Quoted Identifier (using backticks) has a special interpretation for Select expressions(as
Regular Expressions). Have documented current behavior and proposed a solution in attached
doc.
> Summary of solution is:
> Introduce 'standard' quoted identifiers for columns only.
> At the langauage level this is turned on by a flag.
> At the metadata level we relax the constraint on column names.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java fa3e048 
>   itests/qtest/pom.xml 8c249a0 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 3deed45 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java eb26e7f 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java 321759b 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 17e6aad 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g ed9917d 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/ParseDriver.java 1e6826f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java d18ea03 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/UnparseTranslator.java 8fe2262 
>   ql/src/test/queries/clientnegative/invalid_columns.q f8be8c8 
>   ql/src/test/queries/clientpositive/quotedid_alter.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/quotedid_basic.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/quotedid_partition.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/quotedid_skew.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/quotedid_smb.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/quotedid_tblproperty.q PRE-CREATION 
>   ql/src/test/results/clientnegative/invalid_columns.q.out 3311b0a 
>   ql/src/test/results/clientpositive/quotedid_alter.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/quotedid_basic.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/quotedid_partition.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/quotedid_skew.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/quotedid_smb.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/quotedid_tblproperty.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/16299/diff/
> 
> 
> Testing
> -------
> 
> added new tests for create, alter, delete, query with columns containing special characters.
> Tests start with quotedid
> 
> 
> Thanks,
> 
> Harish Butani
> 
>


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