Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1434

User-defined aggregate function that uses a generic interface

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.11.0
    • Component/s: None
    • Labels:
      None

      Description

      AggregateFunctionImpl doesn't work if the class implements a generic interface. We have an interface like below which we want to expose to users for writing aggregate functions.

      public interface UDAF<A, V, R> {
          A init();
          A add(A aggregate, V val);
          R result(A aggregate);
      }
      

      Internally we create an instance of AggregateFunctionImpl and register the Function in SchemaPlus. However this doesn't work for example if we have an implementation like below,

      public class MySum implements UDAF<Number, Number, Number> {
          @Override
          public Number init() {...}
          @Override
          public Number add(Number aggregate, Number val) {...}
          @Override
          public Number result(Number aggregate) {...}
      }
      

      We get an Exception "java.lang.RuntimeException: In user-defined aggregate class 'x.y.z.$MySum', first parameter to 'add' method must be the accumulator (the return type of the 'init' method)"

      This happens because the ReflectiveFunctionBase.findMethod is trying to look for a method name "init" and it get a method with a different signature (that returns Object) instead of the actual method defined in MySum. This happens to be a 'bridge' method inserted by java.

      In findMethod, the bridge methods can be skipped while looking for the method by name.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).
        Hide
        arunmahadevan Arun Mahadevan added a comment - - edited

        Thanks Julian Hyde for pushing the snapshot build. Its really helpful to test out until the 1.11.0 release comes out. Will watch the dev thread as well. Thanks again.

        Show
        arunmahadevan Arun Mahadevan added a comment - - edited Thanks Julian Hyde for pushing the snapshot build. Its really helpful to test out until the 1.11.0 release comes out. Will watch the dev thread as well. Thanks again.
        Hide
        julianhyde Julian Hyde added a comment -

        I've pushed a snapshot based on https://github.com/apache/calcite/commit/d9e9103bb1dec32f41eee8e4d2b4aa11b35a2d5d.

        Releases occur about every 6-8 weeks, but no one has committed to a date right now. If you have opinions when you would like a release, please start a thread on dev.

        Show
        julianhyde Julian Hyde added a comment - I've pushed a snapshot based on https://github.com/apache/calcite/commit/d9e9103bb1dec32f41eee8e4d2b4aa11b35a2d5d . Releases occur about every 6-8 weeks, but no one has committed to a date right now. If you have opinions when you would like a release, please start a thread on dev.
        Hide
        arunmahadevan Arun Mahadevan added a comment - - edited

        Thanks Julian Hyde for accepting the patch. Is there a tentative timeline for 1.11.0 release? If not it would be great if you could push a SNAPSHOT or a bug fix version (for 1.10.0) to maven with this patch.

        Show
        arunmahadevan Arun Mahadevan added a comment - - edited Thanks Julian Hyde for accepting the patch. Is there a tentative timeline for 1.11.0 release? If not it would be great if you could push a SNAPSHOT or a bug fix version (for 1.10.0) to maven with this patch.
        Hide
        julianhyde Julian Hyde added a comment -
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/55ba6b58 . Arun Mahadevan , thanks for the PR!
        Hide
        julianhyde Julian Hyde added a comment -

        I'll take a look.

        Show
        julianhyde Julian Hyde added a comment - I'll take a look.
        Hide
        arunmahadevan Arun Mahadevan added a comment -

        Julian Hyde added a new test case to UdfTest.java.

        Show
        arunmahadevan Arun Mahadevan added a comment - Julian Hyde added a new test case to UdfTest.java.
        Hide
        julianhyde Julian Hyde added a comment -

        Arun Mahadevan, Fix looks good... but we need a test case.

        Show
        julianhyde Julian Hyde added a comment - Arun Mahadevan , Fix looks good... but we need a test case.
        Hide
        arunmahadevan Arun Mahadevan added a comment -

        Raised https://github.com/apache/calcite/pull/308 with potential fix.

        Show
        arunmahadevan Arun Mahadevan added a comment - Raised https://github.com/apache/calcite/pull/308 with potential fix.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            arunmahadevan Arun Mahadevan
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development