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

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.23.0
    • Component/s: 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

            People

            • Assignee:
              Unassigned
              Reporter:
              hyuan Haisheng Yuan

              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

                  Issue deployment