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

AggregateExpandDistinctAggregatesRule should work on Aggregate instead of LogicalAggregate

    Details

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

      Description

      AggregateExpandDistinctAggregatesRule expects the operator to be a subclass of LogicalAggregate:

        public AggregateExpandDistinctAggregatesRule(
            Class<? extends LogicalAggregate> clazz,
            RelFactories.JoinFactory joinFactory) {
          this(clazz, false, RelBuilder.proto(Contexts.of(joinFactory)));
        }
      

      However, LogicalAggregate is a final class which cannot be inherited. This jira proposes to change the signature from LogicalAggregate to Aggregate.

        Activity

        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.13.0 (2017-06-26).

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.13.0 (2017-06-26).
        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/ac364e8c . Thanks for the PR, Haohui Mai !
        Hide
        julianhyde Julian Hyde added a comment -

        Reviewing now. Fixing the comments you missed. Sigh.

        Show
        julianhyde Julian Hyde added a comment - Reviewing now. Fixing the comments you missed. Sigh.
        Hide
        julianhyde Julian Hyde added a comment -

        Please read & consider revising all comments in the file. There are several more occurrences of LogicalAggregate (and LogicalJoin) in comments.

        Show
        julianhyde Julian Hyde added a comment - Please read & consider revising all comments in the file. There are several more occurrences of LogicalAggregate (and LogicalJoin) in comments.
        Hide
        julianhyde Julian Hyde added a comment -

        I saw your PR https://github.com/apache/calcite/pull/447. Please update the comments in that file.

        Show
        julianhyde Julian Hyde added a comment - I saw your PR https://github.com/apache/calcite/pull/447 . Please update the comments in that file.
        Hide
        julianhyde Julian Hyde added a comment -

        Sounds fine. But you should modify the non-deprecated constructor, which takes a RelBuilderFactory argument, rather than the deprecated one which takes a RelFactories.JoinFactory.

        Show
        julianhyde Julian Hyde added a comment - Sounds fine. But you should modify the non-deprecated constructor, which takes a RelBuilderFactory argument, rather than the deprecated one which takes a RelFactories.JoinFactory .
        Hide
        wheat9 Haohui Mai added a comment -

        This issue prevents Flink from applying the same rule to optimize for streaming query, where the streaming aggregate operator is a subclass of Aggregate.

        Show
        wheat9 Haohui Mai added a comment - This issue prevents Flink from applying the same rule to optimize for streaming query, where the streaming aggregate operator is a subclass of Aggregate .

          People

          • Assignee:
            wheat9 Haohui Mai
            Reporter:
            wheat9 Haohui Mai
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development