You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jason Dere <jd...@hortonworks.com> on 2014/01/29 11:29:41 UTC
Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17494/
-----------------------------------------------------------
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.
Diffs
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1b01238
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 9ad5986
metastore/if/hive_metastore.thrift e4e816d
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 58f9957
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java ed05790
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 4288781
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 794fadd
metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java f5ea2ef
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 003dc9c
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java 6dd0852
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 d32deea
ql/src/java/org/apache/hadoop/hive/ql/parse/FunctionSemanticAnalyzer.java da917f7
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 216c361
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
Diff: https://reviews.apache.org/r/17494/diff/
Testing
-------
Thanks,
Jason Dere
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Jason Dere <jd...@hortonworks.com>.
> 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
>
>
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Ashutosh Chauhan <ha...@apache.org>.
> 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
>
>
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17494/#review34835
-----------------------------------------------------------
Ship it!
Ship It!
- Ashutosh Chauhan
On Feb. 19, 2014, 2:22 a.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17494/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 2:22 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 2552414
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java ca43932
> metastore/if/hive_metastore.thrift 347aad9
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bec21cf
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 6dce0aa
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 6bf1bf1
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 16e189a
> metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 0a76cee
> metastore/src/model/org/apache/hadoop/hive/metastore/model/MFunction.java PRE-CREATION
> metastore/src/model/package.jdo e6aebdd
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java c53a412
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java ed3d7ee
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 92fee9b
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionTask.java 988b389
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionUtils.java 7fd4859
> ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 0b7c128
> ql/src/java/org/apache/hadoop/hive/ql/parse/FunctionSemanticAnalyzer.java cc12f30
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 4d58f96
> ql/src/java/org/apache/hadoop/hive/ql/parse/MacroSemanticAnalyzer.java 36f8f71
> 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/database_drop_not_empty.q.out 6ccf95a
> ql/src/test/results/clientnegative/database_drop_not_empty_restrict.q.out c8275af
> 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 3400ac2
>
> Diff: https://reviews.apache.org/r/17494/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17494/
-----------------------------------------------------------
(Updated Feb. 19, 2014, 2:22 a.m.)
Review request for hive.
Changes
-------
Changes per review from Ashutosh
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 (updated)
-----
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 2552414
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java ca43932
metastore/if/hive_metastore.thrift 347aad9
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java bec21cf
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 6dce0aa
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 6bf1bf1
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 16e189a
metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 0a76cee
metastore/src/model/org/apache/hadoop/hive/metastore/model/MFunction.java PRE-CREATION
metastore/src/model/package.jdo e6aebdd
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java c53a412
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java ed3d7ee
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 92fee9b
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionTask.java 988b389
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionUtils.java 7fd4859
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 0b7c128
ql/src/java/org/apache/hadoop/hive/ql/parse/FunctionSemanticAnalyzer.java cc12f30
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 4d58f96
ql/src/java/org/apache/hadoop/hive/ql/parse/MacroSemanticAnalyzer.java 36f8f71
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/database_drop_not_empty.q.out 6ccf95a
ql/src/test/results/clientnegative/database_drop_not_empty_restrict.q.out c8275af
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 3400ac2
Diff: https://reviews.apache.org/r/17494/diff/
Testing
-------
Thanks,
Jason Dere
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
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.
Changes
-------
Unit test fixes
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 (updated)
-----
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
Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17494/
-----------------------------------------------------------
(Updated Jan. 29, 2014, 10:30 a.m.)
Review request for hive.
Bugs: HIVE-6330
https://issues.apache.org/jira/browse/HIVE-6330
Repository: hive-git
Description (updated)
-------
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 1b01238
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 9ad5986
metastore/if/hive_metastore.thrift e4e816d
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 58f9957
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java ed05790
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 4288781
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 794fadd
metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java f5ea2ef
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 003dc9c
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java 6dd0852
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 d32deea
ql/src/java/org/apache/hadoop/hive/ql/parse/FunctionSemanticAnalyzer.java da917f7
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 216c361
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
Diff: https://reviews.apache.org/r/17494/diff/
Testing
-------
Thanks,
Jason Dere