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-gen.diff
        6 kB
        Edward Capriolo
      2. hive-471.diff
        4 kB
        Edward Capriolo
      3. HIVE-471.6.patch.txt
        23 kB
        Edward Capriolo
      4. HIVE-471.5.patch
        18 kB
        Edward Capriolo
      5. HIVE-471.4.patch
        18 kB
        Edward Capriolo
      6. HIVE-471.3.patch
        17 kB
        Edward Capriolo
      7. HIVE-471.2.patch
        16 kB
        Zheng Shao
      8. HIVE-471.1.patch
        16 kB
        Zheng Shao

        Issue Links

          Activity

          Edward Capriolo created issue -
          Hide
          Edward Capriolo added a comment -

          Patch implements basic reflection features.

          Show
          Edward Capriolo added a comment - Patch implements basic reflection features.
          Edward Capriolo made changes -
          Field Original Value New Value
          Attachment hive-471.diff [ 12407205 ]
          Hide
          Edward Capriolo added a comment -

          Patch adds basic features.

          Show
          Edward Capriolo added a comment - Patch adds basic features.
          Edward Capriolo made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          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
          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
          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?
          Edward Capriolo made changes -
          Attachment hive-471-gen.diff [ 12411456 ]
          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.
          Johan Oskarsson made changes -
          Fix Version/s 0.5.0 [ 12314156 ]
          Fix Version/s 0.4.0 [ 12313714 ]
          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
          Namit Jain made changes -
          Fix Version/s 0.5.0 [ 12314156 ]
          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.
          Zheng Shao made changes -
          Attachment HIVE-471.1.patch [ 12434792 ]
          Zheng Shao made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Zheng Shao made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 0.6.0 [ 12314524 ]
          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 -

          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
          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(...).
          Zheng Shao made changes -
          Attachment HIVE-471.2.patch [ 12435325 ]
          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
          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
          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
          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
          Edward Capriolo made changes -
          Attachment HIVE-471.3.patch [ 12438725 ]
          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 -

          Should be good for trunk.

          Show
          Edward Capriolo added a comment - Should be good for trunk.
          Edward Capriolo made changes -
          Fix Version/s 0.6.0 [ 12314524 ]
          Affects Version/s 0.5.1 [ 12314793 ]
          Affects Version/s 0.6.0 [ 12314524 ]
          Hide
          Paul Yang added a comment -

          +1

          Show
          Paul Yang added a comment - +1
          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
          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
          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?
          Namit Jain made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          John Sichi made changes -
          Fix Version/s 0.6.0 [ 12314524 ]
          Edward Capriolo made changes -
          Attachment HIVE-471.4.patch [ 12449510 ]
          Edward Capriolo made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          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
          Namit Jain made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Edward Capriolo added a comment -

          Almost two years in the making!

          Show
          Edward Capriolo added a comment - Almost two years in the making!
          Edward Capriolo made changes -
          Attachment HIVE-471.5.patch [ 12453072 ]
          Edward Capriolo made changes -
          Fix Version/s 0.7.0 [ 12315150 ]
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 0.6.0 [ 12314524 ]
          Affects Version/s 0.5.1 [ 12314793 ]
          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 ..?
          Namit Jain made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          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 -

          @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
          Edward Capriolo made changes -
          Attachment HIVE-471.6.patch.txt [ 12453366 ]
          Edward Capriolo made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          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 -

          Committed. Thanks Edward

          Show
          Namit Jain added a comment - Committed. Thanks Edward
          Namit Jain made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Carl Steinbach made changes -
          Summary A UDF for simple reflection Add reflect() UDF for reflective invocation of Java methods
          Affects Version/s 0.7.0 [ 12315150 ]
          Affects Version/s 0.6.0 [ 12314524 ]
          Component/s UDF [ 12313585 ]
          Component/s Query Processor [ 12312586 ]
          Carl Steinbach made changes -
          Link This issue relates to HIVE-1877 [ HIVE-1877 ]
          Carl Steinbach made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development