hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Szehon Ho" <sze...@cloudera.com>
Subject Re: Review Request 24962: HIVE-7730: Extend ReadEntity to add accessed columns from query
Date Mon, 25 Aug 2014 18:33:59 GMT


> On Aug. 22, 2014, 6:14 a.m., Szehon Ho wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java, line 54
> > <https://reviews.apache.org/r/24962/diff/1/?file=666753#file666753line54>
> >
> >     Can we make this final, and not have a setter?  The caller can just add to the
list.  It'll make the code a bit simpler.
> >     
> >     Also should it be set?
> 
> Xiaomeng Huang wrote:
>     Thanks, I think it better to be list. I get accessed columns from tableToColumnAccessMap,
which is a Map<String, List<String>>. Hive's native authorization is use this
list too.
>     I get the column list via a table name, then set it to readEntity directly, don't
need to add every one with a loop. so it is necessary to have a setter.
>     BTW, I can also to add a API addAccessedColumn(String column) to add one column to
this column list.
> 
> Szehon Ho wrote:
>     OK its fine if you think it should be list.
>     
>     For the other part, I was thinking to have just one method getAccessedColumn() which
returns list.
>     
>     Then caller (SemanticAnalyzer) can call:   entity.getAccessedColumns().addAll(...).
 The benefit to me is 1) the list can be made final, and 2) make the calling code cleaner
(no need to construct lists and set them).  Also its more consistent with the other collections
in this class.  Hope that makes sense, thanks!
> 
> Xiaomeng Huang wrote:
>     Yes, it fines to me. Fixed it, thanks!

Thanks Xiaomeng, can you please upload the latest to the JIRA for testing and commit?

And I had just one minor comment for your consideration, as it's still not uploaded.  With
this, we dont need to construct a new linkedList for "cols" outside the switches (its kind
of a waste), and we can just directly call entity.getAccessedColumns().addAll(tableToColumnAccessMap.get...),
or you can make a local variable cols = tableToColumnAccessMap.get(...) and then entity.getAccessedColumns().addAll(cols).
 It's not a huge deal, but I was thinking we can fix and upload the patch together.


- Szehon


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


On Aug. 25, 2014, 3:17 a.m., Xiaomeng Huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24962/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2014, 3:17 a.m.)
> 
> 
> Review request for hive, Prasad Mujumdar and Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> External authorization model can not get accessed columns from query. Hive should store
accessed columns to ReadEntity 
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/ReadEntity.java 7ed50b4 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java b05d3b4 
> 
> Diff: https://reviews.apache.org/r/24962/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Xiaomeng Huang
> 
>


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