----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17494/#review34715 -----------------------------------------------------------
metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/17494/#comment64933> Is it better to pass struct Database instead here? metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/17494/#comment64935> 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. metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/17494/#comment64936> Since functionType is defined as enum in java code. May be better to have this as enum here as well. metastore/if/hive_metastore.thrift <https://reviews.apache.org/r/17494/#comment64937> Is this necessary. Can't we use get_functions(dbName, "*") for this one? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/17494/#comment64939> 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. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/17494/#comment64940> Isn't rethrowException() doing same thing? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/17494/#comment64941> newMetaException() can be used here. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/17494/#comment64944> I think this is important. Can you create a follow-up jira for this? metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java <https://reviews.apache.org/r/17494/#comment64946> Wonder if this is needed or can be emulated with getFunctions(dbName, "*")? metastore/src/model/package.jdo <https://reviews.apache.org/r/17494/#comment64947> We also need upgrade script for this new table. Can you create a follow-up jira for this? metastore/src/model/package.jdo <https://reviews.apache.org/r/17494/#comment64954> Since this is enum, may be better to store it as int? metastore/src/model/package.jdo <https://reviews.apache.org/r/17494/#comment64955> As I suggested above, we may also want to store PrincipalType indicating whether owner is a user or role? metastore/src/model/package.jdo <https://reviews.apache.org/r/17494/#comment64949> This is stored as millisSinceEpoch, right? In that case, should this be long? ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java <https://reviews.apache.org/r/17494/#comment64956> There should be an else here, throwing AssertionError, since functionInfo must exist at this point. ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java <https://reviews.apache.org/r/17494/#comment64957> private? ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionTask.java <https://reviews.apache.org/r/17494/#comment64959> Better to use SessionState.getUser(). ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionUtils.java <https://reviews.apache.org/r/17494/#comment64960> UNKNOWN instead of NOT_UDF? ql/src/test/results/clientnegative/drop_func_nonexistent.q.out <https://reviews.apache.org/r/17494/#comment64961> It will be nice to generate better error message here, something along the line of function doesn't exist. - Ashutosh Chauhan 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 > >
