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

RelCollationTraitDef.canConvert should always return true

VotersWatch issueWatchersLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.23.0
    • core

    Description

      CALCITE-1148 introduced the following change to RelCollationTraitDef to fix RelTrait conversion bug, but it is just hiding the underlying issue and adding redundant and unnecessary check to planner.

      @Override public boolean canConvert(RelOptPlanner planner,
            RelCollation fromTrait, RelCollation toTrait, RelNode fromRel) {
          // Returns true only if we can convert.  In this case, we can only convert
          // if the fromTrait (the input) has fields that the toTrait wants to sort.
          for (RelFieldCollation field : toTrait.getFieldCollations()) {
            int index = field.getFieldIndex();
            if (index >= fromRel.getRowType().getFieldCount()) {
              return false;
            }
          }
          return true;
        }
      

      The root cause is that logical operators, especially LogicalSort can have traits, which is a bad design decision, and AggregateReduceFunctionsRule and RelBuilder fails to adjust the column mapping in RelTraitSet. The newly created LogicalProject has collation on column 5 (it just copy its input's RelTraitSet blindly), but it only has 2 columns.

      Attachments

        1. image-2020-03-19-16-44-25-621.png
          649 kB
          Haisheng Yuan
        2. image-2020-03-19-16-47-21-805.png
          59 kB
          Haisheng Yuan

        Issue Links

        Activity

          This comment will be Viewable by All Users Viewable by All Users
          Cancel

          People

            Unassigned Unassigned
            hyuan Haisheng Yuan
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 1h 40m
                1h 40m

                Slack

                  Issue deployment