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

RelDataTypeFactoryImpl.copyType() did not copy StructKind

    Details

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

      Description

      Now that StructKind has been introduced as an attribute of RelDataType by CALCITE-1208, when copying a RecordType, we should also copy its StructKind.
      This would be an issue in view expansion, in which columns could not be resolved from a view the same way as they would from its original table.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          I can imagine that a SQL test is hard. But maybe you could call copyType within a test that uses a MockCatalogReader, and check the resulting type?

          Show
          julianhyde Julian Hyde added a comment - I can imagine that a SQL test is hard. But maybe you could call copyType within a test that uses a MockCatalogReader, and check the resulting type?
          Hide
          maryannxue Maryann Xue added a comment -

          Thank you, Julian Hyde, for the advice! I have pushed in another commit for the PR with a new test case. Would you mind taking a look?

          Show
          maryannxue Maryann Xue added a comment - Thank you, Julian Hyde , for the advice! I have pushed in another commit for the PR with a new test case. Would you mind taking a look?
          Hide
          julianhyde Julian Hyde added a comment -

          In createStructType, could you use type.getFieldNames() and ignoreNullable ? something(type) : getFieldTypeList(type)?

          Test case looks great. +1

          Show
          julianhyde Julian Hyde added a comment - In createStructType , could you use type.getFieldNames() and ignoreNullable ? something(type) : getFieldTypeList(type) ? Test case looks great. +1
          Hide
          maryannxue Maryann Xue added a comment - - edited

          Thank you very much for the review, Julian Hyde! I should definitely use type.getFieldNames(), but when changing to ignoreNullable ? something(type) : RelOptUtil.getFieldTypeList(type), I got a lot of test failures. Then I noticed the comment right above "return createStructType(":

              // REVIEW: angel 18-Aug-2005 dtbug336
              // Shouldn't null refer to the nullability of the record type
              // not the individual field types?
              // For flattening and outer joins, it is desirable to change
              // the nullability of the individual fields.
          

          I guess that was the reason why the test cases failed.

          Show
          maryannxue Maryann Xue added a comment - - edited Thank you very much for the review, Julian Hyde ! I should definitely use type.getFieldNames(), but when changing to ignoreNullable ? something(type) : RelOptUtil.getFieldTypeList(type) , I got a lot of test failures. Then I noticed the comment right above "return createStructType(": // REVIEW: angel 18-Aug-2005 dtbug336 // Shouldn't null refer to the nullability of the record type // not the individual field types? // For flattening and outer joins, it is desirable to change // the nullability of the individual fields. I guess that was the reason why the test cases failed.
          Hide
          julianhyde Julian Hyde added a comment -

          I though "something" would be a new helper method that copies a list of types, making each type nullable. Would that work?

          Show
          julianhyde Julian Hyde added a comment - I though "something" would be a new helper method that copies a list of types, making each type nullable. Would that work?
          Hide
          maryannxue Maryann Xue added a comment -

          This is what I tried but didn't work:

          
              return createStructType(type.getStructKind(),
                  ignoreNullable
                      ? new AbstractList<RelDataType>() {
                        @Override public RelDataType get(int index) {
                          RelDataType fieldType =
                              type.getFieldList().get(index).getType();
                          return copyType(fieldType);
                        }
          
                        @Override public int size() {
                          return type.getFieldCount();
                        }
                      }
                      : RelOptUtil.getFieldTypeList(type),
                  type.getFieldNames());
            }
          

          The "ignoreNullable == true" logic kept the same, as "copyType()", while the "ignoreNullable == false" logic changed from "createTypeWithNullability()" to "RelOptUtil.getFieldTypeList(type)". So I assume the original logic was to apply the nullability of the record type to its field types, instead of using the original nullability of the field types themselves.

          Show
          maryannxue Maryann Xue added a comment - This is what I tried but didn't work: return createStructType(type.getStructKind(), ignoreNullable ? new AbstractList<RelDataType>() { @Override public RelDataType get( int index) { RelDataType fieldType = type.getFieldList().get(index).getType(); return copyType(fieldType); } @Override public int size() { return type.getFieldCount(); } } : RelOptUtil.getFieldTypeList(type), type.getFieldNames()); } The "ignoreNullable == true" logic kept the same, as "copyType()", while the "ignoreNullable == false" logic changed from "createTypeWithNullability()" to "RelOptUtil.getFieldTypeList(type)". So I assume the original logic was to apply the nullability of the record type to its field types, instead of using the original nullability of the field types themselves.
          Hide
          julianhyde Julian Hyde added a comment -

          My mistake, you have to copy. Try this:

              return createStructType(type.getStructKind(),
                  new AbstractList<RelDataType>() {
                    @Override public RelDataType get(int index) {
                      RelDataType fieldType =
                          type.getFieldList().get(index).getType();
                      if (ignoreNullable) {
                        return copyType(fieldType);
                      } else {
                        return createTypeWithNullability(fieldType, nullable);
                      }
                    }
          
                    @Override public int size() {
                      return type.getFieldCount();
                    }
                  },
                  type.getFieldNames());
            }
          
          Show
          julianhyde Julian Hyde added a comment - My mistake, you have to copy. Try this: return createStructType(type.getStructKind(), new AbstractList<RelDataType>() { @Override public RelDataType get( int index) { RelDataType fieldType = type.getFieldList().get(index).getType(); if (ignoreNullable) { return copyType(fieldType); } else { return createTypeWithNullability(fieldType, nullable); } } @Override public int size() { return type.getFieldCount(); } }, type.getFieldNames()); }
          Hide
          maryannxue Maryann Xue added a comment -

          Yes, that was what worked in the end. Thank you, Julian Hyde! Can I go ahead and check-in with this change?

          Show
          maryannxue Maryann Xue added a comment - Yes, that was what worked in the end. Thank you, Julian Hyde ! Can I go ahead and check-in with this change?
          Hide
          julianhyde Julian Hyde added a comment -

          Yes.

          Show
          julianhyde Julian Hyde added a comment - Yes.
          Show
          maryannxue Maryann Xue added a comment - Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=3b4e171 .
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.11.0 (2017-01-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

            People

            • Assignee:
              maryannxue Maryann Xue
              Reporter:
              maryannxue Maryann Xue
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development