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

AggFunctions supported by JdbcAggregate should depend on SqlKind, instead of operator instance

    Details

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

      Description

      In JdbcAggregate, there is a list of functions that can be implemented and therefore pushed into the underlying database. The list of functions uses operator instance, but it would be better to use SqlKind. This is particularly relevant in case a user creates his own operators (e.g. Drill wraps some of calcite's operators), so comparing instances will not work.

        Activity

        Hide
        minjikim MinJi Kim added a comment -

        Here is my initial patch. Please let me know if there are any concerns. Thanks!

        https://github.com/apache/calcite/pull/262

        Show
        minjikim MinJi Kim added a comment - Here is my initial patch. Please let me know if there are any concerns. Thanks! https://github.com/apache/calcite/pull/262
        Hide
        julianhyde Julian Hyde added a comment -

        Is this a duplicate of CALCITE-1043? Can you build on the patch for that case? (I reviewed the patch as "basically good" but never heard back from Ted Xu.)

        Show
        julianhyde Julian Hyde added a comment - Is this a duplicate of CALCITE-1043 ? Can you build on the patch for that case? (I reviewed the patch as "basically good" but never heard back from Ted Xu .)
        Hide
        minjikim MinJi Kim added a comment -

        I will take a look at CALCITE-1043. From my initial glance, I think CALCITE-1043 might be broader than this JIRA. If that is the case, I will update the patch to address CALCITE-1043.

        Show
        minjikim MinJi Kim added a comment - I will take a look at CALCITE-1043 . From my initial glance, I think CALCITE-1043 might be broader than this JIRA. If that is the case, I will update the patch to address CALCITE-1043 .
        Hide
        minjikim MinJi Kim added a comment -

        Julian Hyde I looked through CALCITE-1043. It looks like patch for CALCITE-1200 (by Venki Korukanti) already made most of the changes in the patch for CALCITE-1043. I pulled in the missing pieces and updated the pull request, https://github.com/apache/calcite/pull/262.

        Show
        minjikim MinJi Kim added a comment - Julian Hyde I looked through CALCITE-1043 . It looks like patch for CALCITE-1200 (by Venki Korukanti ) already made most of the changes in the patch for CALCITE-1043 . I pulled in the missing pieces and updated the pull request, https://github.com/apache/calcite/pull/262 .
        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/6d56222d . Thanks for the PR, MinJi Kim !
        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:
            minjikim MinJi Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development