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 2016/09/22 19:21:26 UTC
Review Request 52171: HIVE-14819 FunctionInfo for permanent functions
shows TEMPORARY FunctionType
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/
-----------------------------------------------------------
Review request for hive and Sergey Shelukhin.
Bugs: HIVE-14819
https://issues.apache.org/jira/browse/HIVE-14819
Repository: hive-git
Description
-------
Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
Diff: https://reviews.apache.org/r/52171/diff/
Testing
-------
Added tests to TestFunctionRegistry
Thanks,
Jason Dere
Re: Review Request 52171: HIVE-14819 FunctionInfo for permanent
functions shows TEMPORARY FunctionType
Posted by Jason Dere <jd...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/
-----------------------------------------------------------
(Updated Sept. 23, 2016, 10:44 p.m.)
Review request for hive and Sergey Shelukhin.
Changes
-------
Adding fix to the issue with FunctionRegistry.isPermanentFunction() caused during reloadFunctions()
Bugs: HIVE-14819
https://issues.apache.org/jira/browse/HIVE-14819
Repository: hive-git
Description
-------
Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java de74c3e
ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
Diff: https://reviews.apache.org/r/52171/diff/
Testing
-------
Added tests to TestFunctionRegistry
Thanks,
Jason Dere
Re: Review Request 52171: HIVE-14819 FunctionInfo for permanent
functions shows TEMPORARY FunctionType
Posted by Jason Dere <jd...@hortonworks.com>.
> On Sept. 22, 2016, 11:53 p.m., Sergey Shelukhin wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java, line 154
> > <https://reviews.apache.org/r/52171/diff/1/?file=1508429#file1508429line154>
> >
> > is isNative just coming from the resistry? I wonder why these two methods exist - you can either choose a type, or not choose a type, for the same registry?
Previously you could not choose a type - the various register methods would automatically set the type to BUILTIN (system registry) or TEMPRORARY (session registry). That was the problem - when registerToSessionRegistry() adds the persistent UDF it has the wrong type.
Trying to keep the old method signatures (which would depend on the default builtin/temporary behavior. Note that the methods to specify the type are set to private, and only used for persistent UDFs.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/#review150102
-----------------------------------------------------------
On Sept. 22, 2016, 7:21 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52171/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 7:21 p.m.)
>
>
> Review request for hive and Sergey Shelukhin.
>
>
> Bugs: HIVE-14819
> https://issues.apache.org/jira/browse/HIVE-14819
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
> ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
> ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
>
> Diff: https://reviews.apache.org/r/52171/diff/
>
>
> Testing
> -------
>
> Added tests to TestFunctionRegistry
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 52171: HIVE-14819 FunctionInfo for permanent
functions shows TEMPORARY FunctionType
Posted by Jason Dere <jd...@hortonworks.com>.
> On Sept. 22, 2016, 11:53 p.m., Sergey Shelukhin wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java, line 499
> > <https://reviews.apache.org/r/52171/diff/1/?file=1508429#file1508429line499>
> >
> > why shouldn't the just be in one?
That's been the behavior since HIVE-2573 - permanent UDFs loaded at Hive initialization are put in the system registry, and added to the session registry once they are used by that session.
I think they at least need to go to the session registry, it's also useful since it can let us know that the UDF resources have already been loaded for that session. If we completely remove the UDF loading behavior in Hive initialization we might be able to remove permanent UDFs from the system registry. But you kept this behavior around in HIVE-13596! :)
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/#review150102
-----------------------------------------------------------
On Sept. 22, 2016, 7:21 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52171/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 7:21 p.m.)
>
>
> Review request for hive and Sergey Shelukhin.
>
>
> Bugs: HIVE-14819
> https://issues.apache.org/jira/browse/HIVE-14819
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
> ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
> ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
>
> Diff: https://reviews.apache.org/r/52171/diff/
>
>
> Testing
> -------
>
> Added tests to TestFunctionRegistry
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 52171: HIVE-14819 FunctionInfo for permanent
functions shows TEMPORARY FunctionType
Posted by Jason Dere <jd...@hortonworks.com>.
> On Sept. 22, 2016, 11:53 p.m., Sergey Shelukhin wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java, line 499
> > <https://reviews.apache.org/r/52171/diff/1/?file=1508429#file1508429line499>
> >
> > why shouldn't the just be in one?
>
> Jason Dere wrote:
> That's been the behavior since HIVE-2573 - permanent UDFs loaded at Hive initialization are put in the system registry, and added to the session registry once they are used by that session.
> I think they at least need to go to the session registry, it's also useful since it can let us know that the UDF resources have already been loaded for that session. If we completely remove the UDF loading behavior in Hive initialization we might be able to remove permanent UDFs from the system registry. But you kept this behavior around in HIVE-13596! :)
Looking at this a bit more, the initial permanent UDF loading mechanism in Hive.reloadFunctions() is actually broken a bit by HIVE-12857 - it's trying to instantiate the UDF class and add it to Registry.persistent. But reloadFunctions() was never intended to actually load all of the UDF jars, so it fails to instantiate the class and reloadFunctions() actually fails to register the function during reloadFunctions(). I'll add a fix to persistent function detection related to this issue.
We can open a followup issue to revisit whether permanent functions should be loaded during Hive initialization.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/#review150102
-----------------------------------------------------------
On Sept. 22, 2016, 7:21 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52171/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 7:21 p.m.)
>
>
> Review request for hive and Sergey Shelukhin.
>
>
> Bugs: HIVE-14819
> https://issues.apache.org/jira/browse/HIVE-14819
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
> ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
> ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
>
> Diff: https://reviews.apache.org/r/52171/diff/
>
>
> Testing
> -------
>
> Added tests to TestFunctionRegistry
>
>
> Thanks,
>
> Jason Dere
>
>
Re: Review Request 52171: HIVE-14819 FunctionInfo for permanent
functions shows TEMPORARY FunctionType
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52171/#review150102
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java (line 153)
<https://reviews.apache.org/r/52171/#comment217952>
is isNative just coming from the resistry? I wonder why these two methods exist - you can either choose a type, or not choose a type, for the same registry?
ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java (line 498)
<https://reviews.apache.org/r/52171/#comment217955>
why shouldn't the just be in one?
- Sergey Shelukhin
On Sept. 22, 2016, 7:21 p.m., Jason Dere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52171/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 7:21 p.m.)
>
>
> Review request for hive and Sergey Shelukhin.
>
>
> Bugs: HIVE-14819
> https://issues.apache.org/jira/browse/HIVE-14819
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Patch to allow the registry to set PERSISTENT type when registering permanent functions to the session registry. Previously all functions added to session registry had the TEMPORARY tag
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionInfo.java 30ba996
> ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java 05926b5
> ql/src/java/org/apache/hadoop/hive/ql/exec/WindowFunctionInfo.java a16d9e5
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java 911b86b
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java d2d5a1b
>
> Diff: https://reviews.apache.org/r/52171/diff/
>
>
> Testing
> -------
>
> Added tests to TestFunctionRegistry
>
>
> Thanks,
>
> Jason Dere
>
>