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

User-defined aggregate functions with more than one parameter

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.12.0
    • Fix Version/s: 1.14.0
    • Component/s: core
    • Labels:
      None

      Description

      Currently user-defined aggregate functions must have one parameter.

      SqlAggFunction need new constructor to set paramTypes to allow user-defined aggregate function to have more than one parameter.

      If a class(A) implement interface of AggregateFunction,that instance of class will be transformed to instance of SqlUserDefinedAggFunction by call toOp method in CalciteCatalogReader.
      when construct the SqlUserDefinedAggFunction instance will set paramTypes to null for super class sqlFunction indirectly.
      The problem is:
      we will get SqlUserDefinedAggFunction paramTypes to sql validatefilte,but it will be filter nothing

        Issue Links

          Activity

          Hide
          yuemeng yuemeng added a comment - - edited
           private static Iterator<SqlOperator> filterRoutinesByParameterType(
                SqlSyntax syntax,
                final Iterator<SqlOperator> routines,
                final List<RelDataType> argTypes, final List<String> argNames) {
              if (syntax != SqlSyntax.FUNCTION) {
                return routines;
              }
          
              //noinspection unchecked
              return (Iterator) Iterators.filter(
                  Iterators.filter(routines, SqlFunction.class),
                  new PredicateImpl<SqlFunction>() {
                    public boolean test(SqlFunction function) {
                      List<RelDataType> paramTypes = function.getParamTypes();
                      if (paramTypes == null) {
                        // no parameter information for builtins; keep for now
                        return true;
                      }
          

          because function is SqlUserDefinedAggFunction instance ,the result of function.getParamTypes() will always null, so it filters nothing by filterRoutinesByParameterType function

          Show
          yuemeng yuemeng added a comment - - edited private static Iterator<SqlOperator> filterRoutinesByParameterType( SqlSyntax syntax, final Iterator<SqlOperator> routines, final List<RelDataType> argTypes, final List< String > argNames) { if (syntax != SqlSyntax.FUNCTION) { return routines; } //noinspection unchecked return (Iterator) Iterators.filter( Iterators.filter(routines, SqlFunction.class), new PredicateImpl<SqlFunction>() { public boolean test(SqlFunction function) { List<RelDataType> paramTypes = function.getParamTypes(); if (paramTypes == null ) { // no parameter information for builtins; keep for now return true ; } because function is SqlUserDefinedAggFunction instance ,the result of function.getParamTypes() will always null, so it filters nothing by filterRoutinesByParameterType function
          Hide
          julianhyde Julian Hyde added a comment -

          Can you explain this in terms that would make sense to the end user? Does "Allow user-defined aggregate function to have more than one parameter" capture it?

          Show
          julianhyde Julian Hyde added a comment - Can you explain this in terms that would make sense to the end user? Does "Allow user-defined aggregate function to have more than one parameter" capture it?
          Hide
          yuemeng yuemeng added a comment -

          Julian Hyde can u review this issue
          For now,calcite can't support user-defined aggregate function to have more than one parameter

          Show
          yuemeng yuemeng added a comment - Julian Hyde can u review this issue For now,calcite can't support user-defined aggregate function to have more than one parameter
          Hide
          julianhyde Julian Hyde added a comment -

          OK, I'll change the bug abstract. If you contribute a pull request with tests for multi-parameter user-defined aggregate functions (something similar to UdfTest.testUserDefinedAggregateFunction) I will review it.

          Show
          julianhyde Julian Hyde added a comment - OK, I'll change the bug abstract. If you contribute a pull request with tests for multi-parameter user-defined aggregate functions (something similar to UdfTest.testUserDefinedAggregateFunction ) I will review it.
          Hide
          yuemeng yuemeng added a comment -

          Julian Hyde,hi, can u assign this issue to me,I will contribute a pull request with tests

          Show
          yuemeng yuemeng added a comment - Julian Hyde ,hi, can u assign this issue to me,I will contribute a pull request with tests
          Hide
          julianhyde Julian Hyde added a comment -

          Done.

          Show
          julianhyde Julian Hyde added a comment - Done.
          Hide
          yuemeng yuemeng added a comment -

          ok.
          thanks Julian Hyde

          Show
          yuemeng yuemeng added a comment - ok. thanks Julian Hyde
          Hide
          yuemeng yuemeng added a comment -

          Julian Hyde can you help me to review this pr,thanks

          Show
          yuemeng yuemeng added a comment - Julian Hyde can you help me to review this pr,thanks
          Hide
          yuemeng yuemeng added a comment -

          Julian Hyde,can you review this pull request?

          Show
          yuemeng yuemeng added a comment - Julian Hyde ,can you review this pull request?
          Hide
          julianhyde Julian Hyde added a comment -

          I'm on vacation until Sunday. Ask on the dev list; maybe someone else can review it.

          Show
          julianhyde Julian Hyde added a comment - I'm on vacation until Sunday. Ask on the dev list; maybe someone else can review it.
          Hide
          julianhyde Julian Hyde added a comment -

          Can you please convert the patch to a github pull-request, and I will review?

          Show
          julianhyde Julian Hyde added a comment - Can you please convert the patch to a github pull-request, and I will review?
          Hide
          yuemeng yuemeng added a comment -
          Show
          yuemeng yuemeng added a comment - Julian Hyde cau reivew this pr,thanks. https://github.com/apache/calcite/pull/481
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing and testing now. I had to rebase (your PR is off of quite an old branch) and I had to fix two test messages.

          Show
          julianhyde Julian Hyde added a comment - Reviewing and testing now. I had to rebase (your PR is off of quite an old branch) and I had to fix two test messages.
          Hide
          julianhyde Julian Hyde added a comment -

          What name shall I put on the commit? Your github says "hzyuemeng1" but jira says "yuemeng".

          Show
          julianhyde Julian Hyde added a comment - What name shall I put on the commit? Your github says "hzyuemeng1" but jira says "yuemeng".
          Hide
          yuemeng yuemeng added a comment - - edited

          Julian Hyde
          Firstly,thanks a lot for your review.
          since ,our project based on the latest release version 1.12.0,I fixed this bug based on that version,the master can solve this bug use the same way
          you can put hzyuemeng1 on the commit.
          thanks for your review and advise

          Show
          yuemeng yuemeng added a comment - - edited Julian Hyde Firstly,thanks a lot for your review. since ,our project based on the latest release version 1.12.0,I fixed this bug based on that version,the master can solve this bug use the same way you can put hzyuemeng1 on the commit. thanks for your review and advise
          Hide
          yuemeng yuemeng added a comment -

          hi,Julian Hyde

          If necessary,i can put a new PR to master

          Show
          yuemeng yuemeng added a comment - hi, Julian Hyde If necessary,i can put a new PR to master
          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/47c8df9e . Thanks for the PR, yuemeng !
          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development