hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ashutosh Chauhan" <hashut...@apache.org>
Subject Re: Review Request 17494: HIVE-6330: Metastore support for permanent UDFs
Date Tue, 18 Feb 2014 22:41:58 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message