Hive
  1. Hive
  2. HIVE-471

Add reflect() UDF for reflective invocation of Java methods

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.7.0
    • Fix Version/s: 0.7.0
    • Component/s: UDF
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      There are many methods in java that are static and have no arguments or can be invoked with one simple parameter. More complicated functions will require a UDF but one generic one can work as a poor-mans UDF.

      SELECT reflect("java.lang.String", "valueOf", 1), reflect("java.lang.String", "isEmpty")
      FROM src LIMIT 1;
      
      1. HIVE-471.6.patch.txt
        23 kB
        Edward Capriolo
      2. HIVE-471.5.patch
        18 kB
        Edward Capriolo
      3. HIVE-471.4.patch
        18 kB
        Edward Capriolo
      4. HIVE-471.3.patch
        17 kB
        Edward Capriolo
      5. HIVE-471.2.patch
        16 kB
        Zheng Shao
      6. HIVE-471.1.patch
        16 kB
        Zheng Shao
      7. hive-471-gen.diff
        6 kB
        Edward Capriolo
      8. hive-471.diff
        4 kB
        Edward Capriolo

        Issue Links

          Activity

          Hide
          Namit Jain added a comment -

          Committed. Thanks Edward

          Show
          Namit Jain added a comment - Committed. Thanks Edward
          Hide
          Namit Jain added a comment -

          +1

          will commit if the tests pass

          Show
          Namit Jain added a comment - +1 will commit if the tests pass
          Hide
          Namit Jain added a comment -

          @Edward, negative tests are good. We are still adding a lot of them

          Show
          Namit Jain added a comment - @Edward, negative tests are good. We are still adding a lot of them
          Hide
          Edward Capriolo added a comment -

          @Namit,
          I thought we were getting rid of negative tests. Am I misinformed?

          Show
          Edward Capriolo added a comment - @Namit, I thought we were getting rid of negative tests. Am I misinformed?
          Hide
          Namit Jain added a comment -

          Yes, probably the longest time for a patch.

          Few minor comments:

          1. Can you add test for desc and desc extended reflect ?
          2. Can you add some negative test cases - the function does not exist, the parameters are wrong ..?

          Show
          Namit Jain added a comment - Yes, probably the longest time for a patch. Few minor comments: 1. Can you add test for desc and desc extended reflect ? 2. Can you add some negative test cases - the function does not exist, the parameters are wrong ..?
          Hide
          Edward Capriolo added a comment -

          Almost two years in the making!

          Show
          Edward Capriolo added a comment - Almost two years in the making!
          Hide
          Namit Jain added a comment -

          @Edward, can you regenerate the patch - this has been hanging for a long time, and we should try to get it in

          Show
          Namit Jain added a comment - @Edward, can you regenerate the patch - this has been hanging for a long time, and we should try to get it in
          Hide
          Paul Yang added a comment -

          Almost forgot this one - show_functions.q will fail because we added a new function. @Edward, can you re-run show_functions.q and overwrite the test output?

          Show
          Paul Yang added a comment - Almost forgot this one - show_functions.q will fail because we added a new function. @Edward, can you re-run show_functions.q and overwrite the test output?
          Hide
          Namit Jain added a comment -

          @Edward, can you regenerate the patch - I am getting some conflicts ?

          Show
          Namit Jain added a comment - @Edward, can you regenerate the patch - I am getting some conflicts ?
          Hide
          Namit Jain added a comment -

          Will commit if the tests pass

          Show
          Namit Jain added a comment - Will commit if the tests pass
          Hide
          Paul Yang added a comment -

          +1

          Show
          Paul Yang added a comment - +1
          Hide
          Edward Capriolo added a comment -

          Should be good for trunk.

          Show
          Edward Capriolo added a comment - Should be good for trunk.
          Hide
          Edward Capriolo added a comment -

          Can we go +1 on this? I have three sets of UDFs I want to follow up with and since they all share the FunctionRegistry.java. They will all conflict.

          Show
          Edward Capriolo added a comment - Can we go +1 on this? I have three sets of UDFs I want to follow up with and since they all share the FunctionRegistry.java. They will all conflict.
          Hide
          Edward Capriolo added a comment -

          Patch adds apache header, adds annotations for description, adds annotation for Deterministic=false

          Show
          Edward Capriolo added a comment - Patch adds apache header, adds annotations for description, adds annotation for Deterministic=false
          Hide
          Zheng Shao added a comment -

          Thanks Paul and Edward.
          Edward, please proceed with the annotations and commit when you get time.

          Show
          Zheng Shao added a comment - Thanks Paul and Edward. Edward, please proceed with the annotations and commit when you get time.
          Hide
          Paul Yang added a comment -

          Got it, thanks for the clarification. Other than that, the patch looks good.

          Show
          Paul Yang added a comment - Got it, thanks for the clarification. Other than that, the patch looks good.
          Hide
          Edward Capriolo added a comment -

          @Zheng,

          Thank you for following up on this UDF. my version internally but this version looks/ is superior in most ways. I will test it out and do my own commit. I will also add the annotations I think all the UDF's need.

          Ed

          Show
          Edward Capriolo added a comment - @Zheng, Thank you for following up on this UDF. my version internally but this version looks/ is superior in most ways. I will test it out and do my own commit. I will also add the annotations I think all the UDF's need. Ed
          Hide
          Zheng Shao added a comment -

          This patch adds comments to explain why we use ObjectInspectorUtils.compare(...).

          Show
          Zheng Shao added a comment - This patch adds comments to explain why we use ObjectInspectorUtils.compare(...).
          Hide
          Zheng Shao added a comment -

          In most cases, the class name/method name will be passed in as a Text (StringWritable) or a Java String (there can be other cases but relatively rare).
          Also, the class name/method name usually do not change.

          With the current implementation, we avoid doing object creation for most of the time (the only function called is ObjectInspectorUtils.compare()).
          If we convert the name to String and compare with String.equals, then we have to do an object conversion (which may involve object creation) for each of the rows.

          I will add these as the comments in the code. Thanks for asking. That's a good question to answer in the code comments.

          Show
          Zheng Shao added a comment - In most cases, the class name/method name will be passed in as a Text (StringWritable) or a Java String (there can be other cases but relatively rare). Also, the class name/method name usually do not change. With the current implementation, we avoid doing object creation for most of the time (the only function called is ObjectInspectorUtils.compare()). If we convert the name to String and compare with String.equals, then we have to do an object conversion (which may involve object creation) for each of the rows. I will add these as the comments in the code. Thanks for asking. That's a good question to answer in the code comments.
          Hide
          Paul Yang added a comment -

          Question about the patch - for className and methodName in evaluate(), it looks like we are making a copy of the object and the object inspector so we can use ObjectInspectorUtils.compare(). But since we know that the class name and method name arguments are inspectable by a StringObjectInspector, can't we save the the name of the class/method as a java string and check for equality with '=='?

          Show
          Paul Yang added a comment - Question about the patch - for className and methodName in evaluate(), it looks like we are making a copy of the object and the object inspector so we can use ObjectInspectorUtils.compare(). But since we know that the class name and method name arguments are inspectable by a StringObjectInspector, can't we save the the name of the class/method as a java string and check for equality with '=='?
          Hide
          Zheng Shao added a comment -

          Hi Edward, I modified your patch to make it work.

          Can you take a look and let me know any questions? It will be great to add enough comments in the code to make it clear to other developers.

          Show
          Zheng Shao added a comment - Hi Edward, I modified your patch to make it work. Can you take a look and let me know any questions? It will be great to add enough comments in the code to make it clear to other developers.
          Hide
          Namit Jain added a comment -

          It would be good to get it in - but it is not a blocker for 0.5

          Show
          Namit Jain added a comment - It would be good to get it in - but it is not a blocker for 0.5
          Hide
          Ashish Thusoo added a comment -

          Sorry in the delay in responding. Zheng would be able to give you better answers here.

          For the error that you are getting I think you have to end up using ObjectInspectorConvertors I think but Zheng can confirm.

          For different return type you could take a look at the GenericUDFCase.java and look at how it defines ReturnObjectInspectorResolver. Similar to that you could have a ReflectionObjectInspectorResolver which returns the proper ObjectInspector corresponding to the return type.

          ObjectInspectors essentially encode the type and the in memory formats of the object. Let us know if you are able to make progress by following GenericUDFCase.java.

          Show
          Ashish Thusoo added a comment - Sorry in the delay in responding. Zheng would be able to give you better answers here. For the error that you are getting I think you have to end up using ObjectInspectorConvertors I think but Zheng can confirm. For different return type you could take a look at the GenericUDFCase.java and look at how it defines ReturnObjectInspectorResolver. Similar to that you could have a ReflectionObjectInspectorResolver which returns the proper ObjectInspector corresponding to the return type. ObjectInspectors essentially encode the type and the in memory formats of the object. Let us know if you are able to make progress by following GenericUDFCase.java.
          Hide
          Edward Capriolo added a comment -

          I updated the reflection UDF to attempt to use GenericUDF.

          I have a couple questions and I can use suggestions:

               Text t = new Text();
                t.set(""+result);
                return t;
          

          I am currently doing this. I was under the impression that a GenericUDF can return anything. However my return is based on the output of reflection. It could be any type including void. How can I control the return type.

          reflect("java.lang.String", "valueOf", 1)
          

          In this case the reflection fails because of a Hive conversion.
          no method java.lang.NoSuchMethodException: java.lang.String.valueOf(org.apache.hadoop.io.IntWritable)

          Should I write a function to do some auto boxing? If so, is there any predefined conversions I can use as a guide? What types can I expect and so on?

          Show
          Edward Capriolo added a comment - I updated the reflection UDF to attempt to use GenericUDF. I have a couple questions and I can use suggestions: Text t = new Text(); t.set(""+result); return t; I am currently doing this. I was under the impression that a GenericUDF can return anything. However my return is based on the output of reflection. It could be any type including void. How can I control the return type. reflect("java.lang.String", "valueOf", 1) In this case the reflection fails because of a Hive conversion. no method java.lang.NoSuchMethodException: java.lang.String.valueOf(org.apache.hadoop.io.IntWritable) Should I write a function to do some auto boxing? If so, is there any predefined conversions I can use as a guide? What types can I expect and so on?
          Hide
          Zheng Shao added a comment -

          One thing for improvement is that the reflection should NOT be done on a per-row basis. We should do it once and save the Method (on the first invocation).

          Show
          Zheng Shao added a comment - One thing for improvement is that the reflection should NOT be done on a per-row basis. We should do it once and save the Method (on the first invocation).
          Hide
          Ashish Thusoo added a comment -

          nice idea. I like the concept.

          You probably should use GenericUDF framework instead of the UDF framework. That way you will be easily able to pass variable number of arguments to the function.

          Show
          Ashish Thusoo added a comment - nice idea. I like the concept. You probably should use GenericUDF framework instead of the UDF framework. That way you will be easily able to pass variable number of arguments to the function.
          Hide
          Edward Capriolo added a comment -

          Patch adds basic features.

          Show
          Edward Capriolo added a comment - Patch adds basic features.
          Hide
          Edward Capriolo added a comment -

          Patch implements basic reflection features.

          Show
          Edward Capriolo added a comment - Patch implements basic reflection features.

            People

            • Assignee:
              Edward Capriolo
              Reporter:
              Edward Capriolo
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development