Hive
  1. Hive
  2. HIVE-5203

FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: Types, UDF
    • Labels:
      None

      Description

      When the function registry is trying to determine the best version of UDF evaluate() to use based on a set of arguments passed in, it should prefer methods where the argument types are more related to the original types. For example if varchar is used with UDFFromUnixTime(), varchar is convertible to both the double and string versions of evaluate() for that UDF. In this case we would prefer that the function registry select the string version over the double version, since varchar and string are both string types.

      This doesn't really affect any of the existing types, but comes into play with the addition of the varchar type (HIVE-4844).

      1. HIVE-5203.1.patch
        5 kB
        Jason Dere
      2. HIVE-5203.2.patch
        13 kB
        Jason Dere

        Issue Links

          Activity

          Hide
          Jason Dere added a comment -

          Attaching HIVE-5203.1.patch

          Show
          Jason Dere added a comment - Attaching HIVE-5203 .1.patch
          Hide
          Edward Capriolo added a comment -

          Would you mind writing some standard junit tests on this one?

          Show
          Edward Capriolo added a comment - Would you mind writing some standard junit tests on this one?
          Hide
          Jason Dere added a comment -

          I have a test case in the main varchar patch, but it won't work here since that type doesn't exist yet. Unfortunately with the compatibility behavior between the existing types, I don't think we can actually hit a situation where we'll run into this issue with the current types. The closest we'd be able to get would be if Date and Timestamp were implicitly convertable to one another, and they are not. Otherwise, the best we can do at the moment is testing that the existing tests in TestFunctionRegistry still work fine.

          Show
          Jason Dere added a comment - I have a test case in the main varchar patch, but it won't work here since that type doesn't exist yet. Unfortunately with the compatibility behavior between the existing types, I don't think we can actually hit a situation where we'll run into this issue with the current types. The closest we'd be able to get would be if Date and Timestamp were implicitly convertable to one another, and they are not. Otherwise, the best we can do at the moment is testing that the existing tests in TestFunctionRegistry still work fine.
          Hide
          Hive QA added a comment -

          Overall: -1 at least one tests failed

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12601280/HIVE-5203.1.patch

          ERROR: -1 due to 1 failed/errored test(s), 2909 tests executed
          Failed tests:

          org.apache.hcatalog.mapreduce.TestHCatExternalPartitioned.testHCatPartitionedTable
          

          Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/603/testReport
          Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/603/console

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          Tests failed with: TestsFailedException: 1 tests failed
          

          This message is automatically generated.

          Show
          Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12601280/HIVE-5203.1.patch ERROR: -1 due to 1 failed/errored test(s), 2909 tests executed Failed tests: org.apache.hcatalog.mapreduce.TestHCatExternalPartitioned.testHCatPartitionedTable Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/603/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/603/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests failed with: TestsFailedException: 1 tests failed This message is automatically generated.
          Hide
          Ashutosh Chauhan added a comment -

          Jason's rationale for this change and subsequent explanation sounds reasonable to me. +1

          Show
          Ashutosh Chauhan added a comment - Jason's rationale for this change and subsequent explanation sounds reasonable to me. +1
          Hide
          Edward Capriolo added a comment -

          I have a couple more things I think we should do first.

            for (Method m: udfMethods) {
          +        currentScore = 0;
          +        List<TypeInfo> argumentsAccepted =
          +            TypeInfoUtils.getParameterTypeInfos(m, argumentsPassed.size());
          +        Iterator<TypeInfo> argsPassedIter = argumentsPassed.iterator();
          

          Especially this one. We are addign a public method we should have tests around it to document its functionality. java doc would be nice to.

          
          

          public static PrimitiveGrouping getPrimitiveGrouping(PrimitiveCategory primitiveCategory) {
          + switch (primitiveCategory) {

          {public}
          Show
          Edward Capriolo added a comment - I have a couple more things I think we should do first. for (Method m: udfMethods) { + currentScore = 0; + List<TypeInfo> argumentsAccepted = + TypeInfoUtils.getParameterTypeInfos(m, argumentsPassed.size()); + Iterator<TypeInfo> argsPassedIter = argumentsPassed.iterator(); Especially this one. We are addign a public method we should have tests around it to document its functionality. java doc would be nice to. public static PrimitiveGrouping getPrimitiveGrouping(PrimitiveCategory primitiveCategory) { + switch (primitiveCategory) { {public}
          Hide
          Jason Dere added a comment -

          Hi Edward, did you have a particular comment around the first code section you highlighted in your previous comment? It looks like your comments were directed against the 2nd code section.

           for (Method m: udfMethods) {
          +        currentScore = 0;
          +        List<TypeInfo> argumentsAccepted =
          +            TypeInfoUtils.getParameterTypeInfos(m, argumentsPassed.size());
          +        Iterator<TypeInfo> argsPassedIter = argumentsPassed.iterator();
          
          Show
          Jason Dere added a comment - Hi Edward, did you have a particular comment around the first code section you highlighted in your previous comment? It looks like your comments were directed against the 2nd code section. for (Method m: udfMethods) { + currentScore = 0; + List<TypeInfo> argumentsAccepted = + TypeInfoUtils.getParameterTypeInfos(m, argumentsPassed.size()); + Iterator<TypeInfo> argsPassedIter = argumentsPassed.iterator();
          Hide
          Edward Capriolo added a comment -

          I was thinking you could use mock up a class with a few methods with signatures you are looking to hit.

            MyTestClass {
              eval(String)
              eval(Text)
              eval(int)
            }
          

          Then use that code in a test case and maybe use mockito to show that the logic is finding the method you want.

          Really as long as it continues to work the same way (current tests) we know it is correct, but it would be nice to have a more direct way to show how this logic works (with a unit test). Just a thought.

          Show
          Edward Capriolo added a comment - I was thinking you could use mock up a class with a few methods with signatures you are looking to hit. MyTestClass { eval( String ) eval(Text) eval( int ) } Then use that code in a test case and maybe use mockito to show that the logic is finding the method you want. Really as long as it continues to work the same way (current tests) we know it is correct, but it would be nice to have a more direct way to show how this logic works (with a unit test). Just a thought.
          Hide
          Jason Dere added a comment -

          Attaching HIVE-5203.2.patch. This breaks out the type affinity-based selection to a separate method for better testability, and adds tests.

          Show
          Jason Dere added a comment - Attaching HIVE-5203 .2.patch. This breaks out the type affinity-based selection to a separate method for better testability, and adds tests.
          Hide
          Jason Dere added a comment -

          Managed to add some tests by moving the new functionality to a separate method, though we'd never hit those scenarios in real life. Also posted the changes at https://reviews.facebook.net/D12711

          Show
          Jason Dere added a comment - Managed to add some tests by moving the new functionality to a separate method, though we'd never hit those scenarios in real life. Also posted the changes at https://reviews.facebook.net/D12711
          Hide
          Hive QA added a comment -

          Overall: -1 at least one tests failed

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12601464/HIVE-5203.2.patch

          ERROR: -1 due to 4 failed/errored test(s), 2911 tests executed
          Failed tests:

          org.apache.hcatalog.pig.TestHCatLoader.testGetInputBytes
          org.apache.hcatalog.pig.TestHCatLoader.testReadPartitionedBasic
          org.apache.hcatalog.pig.TestHCatLoader.testProjectionsBasic
          org.apache.hcatalog.mapreduce.TestHCatExternalDynamicPartitioned.testHCatDynamicPartitionedTableMultipleTask
          

          Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/616/testReport
          Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/616/console

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          Tests failed with: TestsFailedException: 4 tests failed
          

          This message is automatically generated.

          Show
          Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12601464/HIVE-5203.2.patch ERROR: -1 due to 4 failed/errored test(s), 2911 tests executed Failed tests: org.apache.hcatalog.pig.TestHCatLoader.testGetInputBytes org.apache.hcatalog.pig.TestHCatLoader.testReadPartitionedBasic org.apache.hcatalog.pig.TestHCatLoader.testProjectionsBasic org.apache.hcatalog.mapreduce.TestHCatExternalDynamicPartitioned.testHCatDynamicPartitionedTableMultipleTask Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/616/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/616/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests failed with: TestsFailedException: 4 tests failed This message is automatically generated.
          Hide
          Jason Dere added a comment -

          Ran both TestHCatLoader and TestHCatExternalDynamicPartitioned locally and these tests pass for me.

          Show
          Jason Dere added a comment - Ran both TestHCatLoader and TestHCatExternalDynamicPartitioned locally and these tests pass for me.
          Hide
          Edward Capriolo added a comment -

          Cool looks good +1

          Show
          Edward Capriolo added a comment - Cool looks good +1
          Hide
          Ashutosh Chauhan added a comment -

          Committed to trunk. Thanks, Jason!

          Show
          Ashutosh Chauhan added a comment - Committed to trunk. Thanks, Jason!
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hive-trunk-h0.21 #2313 (See https://builds.apache.org/job/Hive-trunk-h0.21/2313/)
          HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413)

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java
          • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hive-trunk-h0.21 #2313 (See https://builds.apache.org/job/Hive-trunk-h0.21/2313/ ) HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413 ) /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hive-trunk-hadoop2 #408 (See https://builds.apache.org/job/Hive-trunk-hadoop2/408/)
          HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413)

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java
          • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hive-trunk-hadoop2 #408 (See https://builds.apache.org/job/Hive-trunk-hadoop2/408/ ) HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413 ) /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hive-trunk-hadoop1-ptest #152 (See https://builds.apache.org/job/Hive-trunk-hadoop1-ptest/152/)
          HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413)

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java
          • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hive-trunk-hadoop1-ptest #152 (See https://builds.apache.org/job/Hive-trunk-hadoop1-ptest/152/ ) HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413 ) /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hive-trunk-hadoop2-ptest #85 (See https://builds.apache.org/job/Hive-trunk-hadoop2-ptest/85/)
          HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413)

          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java
          • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive
          • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hive-trunk-hadoop2-ptest #85 (See https://builds.apache.org/job/Hive-trunk-hadoop2-ptest/85/ ) HIVE-5203 : FunctionRegistry.getMethodInternal() should prefer method arguments with closer affinity to the original argument types (Jason Dere via Ashutosh Chauhan) (hashutosh: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1520413 ) /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/primitive/TestPrimitiveObjectInspectorUtils.java
          Hide
          Ashutosh Chauhan added a comment -

          This issue has been fixed and released as part of 0.12 release. If you find further issues, please create a new jira and link it to this one.

          Show
          Ashutosh Chauhan added a comment - This issue has been fixed and released as part of 0.12 release. If you find further issues, please create a new jira and link it to this one.

            People

            • Assignee:
              Jason Dere
              Reporter:
              Jason Dere
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development