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

Create separate SqlFunctionCategory values for table functions and macros

    Details

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

      Description

      This avoids trying to apply a table function in a context where it does not apply or a regular function where a table function is needed.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          I presume that you want to add SqlFunctionCategory values because SqlOperatorTable.lookupOperatorOverloads is your interface of choice? But it doesn't help with Schema.getFunctions(String), right?

          Some review comments:

          • How about converting SqlFunctionCategory.isSpecific() (and similar methods) into a final boolean field?
          • isUnresolvedUserDefinedFunction is misleadingly named.
          • isUserdefined is mis-spelled.
          • Why would anyone create a SqlUserDefinedFunction whose category is not USER_DEFINED_FUNCTION?
          • In CalciteCatalogReader can you use Iterables.filter? Iterator.remove always gives me the creeps.
          • Please run checkstyle and fix the format of the commit string.
          Show
          julianhyde Julian Hyde added a comment - I presume that you want to add SqlFunctionCategory values because SqlOperatorTable.lookupOperatorOverloads is your interface of choice? But it doesn't help with Schema.getFunctions(String), right? Some review comments: How about converting SqlFunctionCategory.isSpecific() (and similar methods) into a final boolean field? isUnresolvedUserDefinedFunction is misleadingly named. isUserdefined is mis-spelled. Why would anyone create a SqlUserDefinedFunction whose category is not USER_DEFINED_FUNCTION? In CalciteCatalogReader can you use Iterables.filter ? Iterator.remove always gives me the creeps. Please run checkstyle and fix the format of the commit string.
          Hide
          julienledem Julien Le Dem added a comment - - edited

          Yes, eventually I'd like the FunctionCategory to be passed to getFunctions (or some other mechanism). But this seemed like a reasonable first step.

          • about the boolean field: Yes. However it sometimes becomes hard to read when you have several fields. As everything is defined in the body of the enum I find the current definition more readable. If you like the boolean field better I'll change it.
          • isUnresolvedUserDefinedFunction: I did not have a good name. Suggestions?
          • isUserdefined: Will fix
          • SqlUserDefinedFunction: The existing hierarchy was a little confusing to me. TableUserDefinedFunction extends it. We can change this.
          • Iterables.filter: will do
          • checkstyle : will do
          Show
          julienledem Julien Le Dem added a comment - - edited Yes, eventually I'd like the FunctionCategory to be passed to getFunctions (or some other mechanism). But this seemed like a reasonable first step. about the boolean field: Yes. However it sometimes becomes hard to read when you have several fields. As everything is defined in the body of the enum I find the current definition more readable. If you like the boolean field better I'll change it. isUnresolvedUserDefinedFunction: I did not have a good name. Suggestions? isUserdefined: Will fix SqlUserDefinedFunction: The existing hierarchy was a little confusing to me. TableUserDefinedFunction extends it. We can change this. Iterables.filter: will do checkstyle : will do
          Hide
          julianhyde Julian Hyde added a comment -

          SqlUserDefinedFunction: The existing hierarchy was a little confusing to me. TableUserDefinedFunction extends it. We can change this.

          I don't like to give the end user a choice of many constructors. So maybe the one used (only) by SqlUserDefinedTableFunction could be protected or package-protected so that no one is tempted to use it?

          Show
          julianhyde Julian Hyde added a comment - SqlUserDefinedFunction: The existing hierarchy was a little confusing to me. TableUserDefinedFunction extends it. We can change this. I don't like to give the end user a choice of many constructors. So maybe the one used (only) by SqlUserDefinedTableFunction could be protected or package-protected so that no one is tempted to use it?
          Hide
          julienledem Julien Le Dem added a comment -

          Julian Hyde sorry for the delay. MinJi Kim and I have updated the PR based on your feedback: https://github.com/apache/calcite/pull/168

          Show
          julienledem Julien Le Dem added a comment - Julian Hyde sorry for the delay. MinJi Kim and I have updated the PR based on your feedback: https://github.com/apache/calcite/pull/168
          Hide
          julianhyde Julian Hyde added a comment -

          Will review right after the release.

          Show
          julianhyde Julian Hyde added a comment - Will review right after the release.
          Hide
          julienledem Julien Le Dem added a comment -

          thank you.
          FYI: the build is green
          https://travis-ci.org/apache/calcite/builds/136873629

          Show
          julienledem Julien Le Dem added a comment - thank you. FYI: the build is green https://travis-ci.org/apache/calcite/builds/136873629
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/0599cdde . Julien Le Dem , MinJi Kim , Thanks for the PR!
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              julienledem Julien Le Dem
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development