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 18:26:09 GMT

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


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