> 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?
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. > 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. 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? > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/if/hive_metastore.thrift, line 331 > > <https://reviews.apache.org/r/17494/diff/2/?file=470616#file470616line331> > > > > Since functionType is defined as enum in java code. May be better to > > have this as enum here as well. ok, will fix > 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? Was using Table as example, probably ended with more methods than needed. I can remove get_all_functions(). > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 783 > > <https://reviews.apache.org/r/17494/diff/2/?file=470617#file470617line783> > > > > I think its better to make these checks in different if clause so that > > we can generate better error messages. Currently, users won't know whether > > drop db failed because if it has tables or functions. ok, will fix > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3536 > > <https://reviews.apache.org/r/17494/diff/2/?file=470617#file470617line3536> > > > > Isn't rethrowException() doing same thing? Similar, though I think the return type allowed different types (MetaException, NoSuchObjectException, TException), compared to the exceptions allowed from my method calls. I'll take a look at those methods again, if seems ok to throw all of (MetaException, NoSuchObjectException, TException), then I'll change the methods to allow those exceptions. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3540 > > <https://reviews.apache.org/r/17494/diff/2/?file=470617#file470617line3540> > > > > newMetaException() can be used here. will fix if I keep this method > 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? 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" > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java, line 545 > > <https://reviews.apache.org/r/17494/diff/2/?file=470621#file470621line545> > > > > Wonder if this is needed or can be emulated with getFunctions(dbName, > > "*")? ok, will remove > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/model/package.jdo, line 914 > > <https://reviews.apache.org/r/17494/diff/2/?file=470623#file470623line914> > > > > We also need upgrade script for this new table. Can you create a > > follow-up jira for this? Prasad also brought this up, opened HIVE-6458. > 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? Yeah will change if I also change the thrift def to enum. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/model/package.jdo, line 934 > > <https://reviews.apache.org/r/17494/diff/2/?file=470623#file470623line934> > > > > As I suggested above, we may also want to store PrincipalType > > indicating whether owner is a user or role? Will fix if I add principalType > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > metastore/src/model/package.jdo, line 938 > > <https://reviews.apache.org/r/17494/diff/2/?file=470623#file470623line938> > > > > This is stored as millisSinceEpoch, right? In that case, should this be > > long? createTime for all the metastore objects is stored as integer, and is actually set as secs since epoch, not msec. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java, line 573 > > <https://reviews.apache.org/r/17494/diff/2/?file=470626#file470626line573> > > > > There should be an else here, throwing AssertionError, since > > functionInfo must exist at this point. Actually, I disagree here .. the caller of this method, getFunctionInfo(), is allowed to return null if no function is found, it's on the caller of getFunctionInfo() to do the appropriate error handling. I see this method as being part of the getFunctionInfo() functionality, and thus allowed to return null. > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java, line 632 > > <https://reviews.apache.org/r/17494/diff/2/?file=470626#file470626line632> > > > > private? will fix > 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(). ok, will fix. Do you mean ShimLoader.getHadoopShims().getUGIForConf()? > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionUtils.java, line 79 > > <https://reviews.apache.org/r/17494/diff/2/?file=470628#file470628line79> > > > > UNKNOWN instead of NOT_UDF? sure, will fix > On Feb. 18, 2014, 6:26 p.m., Ashutosh Chauhan wrote: > > ql/src/test/results/clientnegative/drop_func_nonexistent.q.out, line 3 > > <https://reviews.apache.org/r/17494/diff/2/?file=470644#file470644line3> > > > > It will be nice to generate better error message here, something along > > the line of function doesn't exist. Looks like errors are logged to the error logger, but not to the console. Will fix. - Jason ----------------------------------------------------------- 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 > >
