> On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/if/hive_metastore.thrift, line 327 > > <https://reviews.apache.org/r/17494/diff/2/?file=470616#file470616line327> > > > > Is it better to pass struct Database instead here? > > Jason Dere wrote: > Just going by the examples of the other object definitions (table, > partition, index, etc), the objects contain the database name rather than the > database struct. So I think this is correct, unless I've missed something. >
Precedences are not always best to follow :) Seems, like you currently don't need any other db info in metastore. So, its fine for now. But in future if more Database information is required(like owner, location etc), it will be better to pass db object. But, this is useful only if you already have constructed db object with you on client. Otherwise, retrieving db object from metastore and passing it back is wasteful. So, my suggestion is if you already have db object, better to pass it, but if you only have string dbName, than current impl is fine. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/if/hive_metastore.thrift, line 686 > > <https://reviews.apache.org/r/17494/diff/2/?file=470616#file470616line686> > > > > Is this necessary. Can't we use get_functions(dbName, "*") for this one? > > Jason Dere wrote: > Was using Table as example, probably ended with more methods than needed. > I can remove get_all_functions(). > Yeah, lets remove it. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line > > 6271 > > <https://reviews.apache.org/r/17494/diff/2/?file=470620#file470620line6271> > > > > I think this is important. Can you create a follow-up jira for this? > > Jason Dere wrote: > There currently aren't any privileges that can be assigned to functions. > When we add support for that this will need to be addressed. I suppose I can > change this comment to "delete privileges when function privileges are > supported" > Ahhh.. Sometimes comment introduces confusion, instead of helping :). Yeah, lets update comment. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/model/package.jdo, line 929 > > <https://reviews.apache.org/r/17494/diff/2/?file=470623#file470623line929> > > > > Since this is enum, may be better to store it as int? > > Jason Dere wrote: > Yeah will change if I also change the thrift def to enum. > Yeah.. other problem it will help you avoid is what format of string got saved in: Java/JAVA/java. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/if/hive_metastore.thrift, line 329 > > <https://reviews.apache.org/r/17494/diff/2/?file=470616#file470616line329> > > > > Shall we call this ownerName ? > > Additionally, it may make sense to pass enum PrincipalType here as > > well. e.g., Database ownership can belong to either user or role. So, there > > we store this extra enum to distinguish two. > > Jason Dere wrote: > I had been using Table as the example when I had made these changes, > hence the use of owner vs ownerName. I can change to ownerName. So is the > convention for new metastore objects to have this ownerName/principalType > pair? > owner Vs ownerName either is fine with me. Leave it up to you. But, I think including principalType makes sense. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionTask.java, line 146 > > <https://reviews.apache.org/r/17494/diff/2/?file=470627#file470627line146> > > > > Better to use SessionState.getUser(). > > Jason Dere wrote: > ok, will fix. Do you mean ShimLoader.getHadoopShims().getUGIForConf()? > Nopes, SessionState.getUserName(). This is something which got added in last few days. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17494/#review34715 ----------------------------------------------------------- On Feb. 6, 2014, 6:34 a.m., Jason Dere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17494/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2014, 6:34 a.m.) > > > Review request for hive. > > > Bugs: HIVE-6330 > https://issues.apache.org/jira/browse/HIVE-6330 > > > Repository: hive-git > > > Description > ------- > > Metastore changes, CREATE FUNCTION support (without TEMPORARY keyword), > lookup of functions in the metastore. The UDF class needs to be resolvable by > the class loader in Hive. > Generated thrift changes not included in diff. > > > Diffs > ----- > > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java > d7854fe > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 9ad5986 > metastore/if/hive_metastore.thrift e327e2a > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 2d8e483 > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > bcbb52e > metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 377709f > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 0715e22 > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 2e3b6da > metastore/src/model/org/apache/hadoop/hive/metastore/model/MFunction.java > PRE-CREATION > metastore/src/model/package.jdo 49f2aac > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 6998b43 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > f54ae53 > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 48b7ee1 > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionTask.java 988b389 > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionUtils.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java e59decc > ql/src/java/org/apache/hadoop/hive/ql/parse/FunctionSemanticAnalyzer.java > da917f7 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 17f3552 > ql/src/java/org/apache/hadoop/hive/ql/parse/MacroSemanticAnalyzer.java > b42a425 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateFunctionDesc.java 051095a > ql/src/java/org/apache/hadoop/hive/ql/plan/DropFunctionDesc.java 8a78f5b > ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java > 0d02f6e > ql/src/test/queries/clientnegative/create_function_nonexistent_class.q > PRE-CREATION > ql/src/test/queries/clientnegative/create_function_nonexistent_db.q > PRE-CREATION > ql/src/test/queries/clientnegative/create_function_nonudf_class.q > PRE-CREATION > ql/src/test/queries/clientnegative/drop_func_nonexistent.q PRE-CREATION > ql/src/test/queries/clientpositive/create_func1.q PRE-CREATION > ql/src/test/results/clientnegative/create_function_nonexistent_class.q.out > PRE-CREATION > ql/src/test/results/clientnegative/create_function_nonexistent_db.q.out > PRE-CREATION > ql/src/test/results/clientnegative/create_function_nonudf_class.q.out > PRE-CREATION > ql/src/test/results/clientnegative/drop_func_nonexistent.q.out PRE-CREATION > ql/src/test/results/clientpositive/create_func1.q.out PRE-CREATION > ql/src/test/results/clientpositive/show_functions.q.out 57c9036 > > Diff: https://reviews.apache.org/r/17494/diff/ > > > Testing > ------- > > > Thanks, > > Jason Dere > >
