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

Use custom RelBuilder implementation in some rules

    Details

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

      Description

      Some rules do not have constructors with RelBuilderFactory.

      List of such rules that are used in Drill:

      • AggregateRemoveRule
      • ProjectRemoveRule
      • SortRemoveRule
      • ProjectWindowTransposeRule
      • ExpandConversionRule
      • ConverterRule
      • JoinToMultiJoinRule
      • ProjectToWindowRule

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.15.0 (2017-12-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
        Hide
        julianhyde Julian Hyde added a comment -

        Fixed in d407395e. Thanks for the PR, Volodymyr Vysotskyi!

        Show
        julianhyde Julian Hyde added a comment - Fixed in d407395e . Thanks for the PR, Volodymyr Vysotskyi !
        Show
        julianhyde Julian Hyde added a comment - I've reviewed https://github.com/apache/calcite/pull/564/commits/16d786cadcad45430a14a231f39852b1d2f5b735 ; looks good, now testing.
        Hide
        vvysotskyi Volodymyr Vysotskyi added a comment -

        Julian Hyde, thanks for the review. I have addressed all code review comments. Could you please take a look again?

        Show
        vvysotskyi Volodymyr Vysotskyi added a comment - Julian Hyde , thanks for the review. I have addressed all code review comments. Could you please take a look again?
        Hide
        julianhyde Julian Hyde added a comment -

        Reviewing https://github.com/apache/calcite/pull/564/commits/11dd804f982394590a5537e55f6bfce96ac9af7d. It looks good - nice work! A couple of minor things:

        • Where you've added a new constructor, and the old one is public (e.g. AggregateRemoveRule), can you please mark the one one deprecated, "@Deprecated // to be removed before 2.0". We want to keep our API as small as possible. Run "mvn verify" before check-in to make sure that we don't call deprecated methods.
        • When method parameters go onto the next line, our coding style is to indent them 4 spaces, not line them up with the '(' as you have done in some places.
        • If a constructor is now public, make sure it has javadoc.
        Show
        julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/564/commits/11dd804f982394590a5537e55f6bfce96ac9af7d . It looks good - nice work! A couple of minor things: Where you've added a new constructor, and the old one is public (e.g. AggregateRemoveRule), can you please mark the one one deprecated, "@Deprecated // to be removed before 2.0". We want to keep our API as small as possible. Run "mvn verify" before check-in to make sure that we don't call deprecated methods. When method parameters go onto the next line, our coding style is to indent them 4 spaces, not line them up with the '(' as you have done in some places. If a constructor is now public, make sure it has javadoc.
        Hide
        vvysotskyi Volodymyr Vysotskyi added a comment -

        I have created pull request for this Jira: https://github.com/apache/calcite/pull/564
        The first commit contains only changes in rules, listed in Jira description.

        Show
        vvysotskyi Volodymyr Vysotskyi added a comment - I have created pull request for this Jira: https://github.com/apache/calcite/pull/564 The first commit contains only changes in rules, listed in Jira description.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            vvysotskyi Volodymyr Vysotskyi
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development