> 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 > >
