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

Support for modifiable views with extended columns

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.11.0
    • Fix Version/s: 1.12.0
    • Component/s: None
    • Labels:

      Description

      The changes for this PR is to:

      • propagate extended columns (already parsed into namespaces in the validator) through to the planner
      • validate INSERT and UPDATE operations against the constraints of the modifiable view

      Maryann Xue, Rajeshbabu Chintaguntla, James Taylor

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Kevin Liew, I reviewed, and it looks great. Really good test coverage, nice clean code. Only cosmetic issues to fix:

          • Can you change method javadoc comments to 3rd person declarative (e.g. prefer "Gets the label" to "Get the label");
          • Remove the 3 TODOs you added. Either fix the problem, or log a jira case, or remove the comment.
          • There is one checkstyle exception.

          I thought that getExtendedColumns was a bit easier to understand when I added a method to transform the list of alternating names and types into a list of pairs. But it's up to you whether you take that change:

            public static List<RelDataTypeField> getExtendedColumns(
                SqlValidator validator, SqlValidatorTable table, SqlNodeList extendedColumns) {
              final ImmutableList.Builder<RelDataTypeField> extendedFields = ImmutableList.builder();
              final ExtensibleTable extTable = table.unwrap(ExtensibleTable.class);
              int extendedFieldOffset =
                  extTable == null
                      ? table.getRowType().getFieldCount()
                      : extTable.getExtendedColumnOffset();
              for (Pair<SqlIdentifier, SqlDataTypeSpec> pair : pairs(extendedColumns)) {
                final SqlIdentifier identifier = pair.left;
                final SqlDataTypeSpec type = pair.right;
                extendedFields.add(
                    new RelDataTypeFieldImpl(identifier.getSimple(),
                        extendedFieldOffset++,
                        type.deriveType(validator)));
              }
              return extendedFields.build();
            }
          
            /** Converts a list of extended columns
             * (of the form [name0, type0, name1, type1, ...])
             * into a list of (name, type) pairs. */
            private static List<Pair<SqlIdentifier, SqlDataTypeSpec>> pairs(
                SqlNodeList extendedColumns) {
              final List list = extendedColumns.getList();
              //noinspection unchecked
              return Pair.zip(Util.quotientList(list, 2, 0),
                  Util.quotientList(list, 2, 1));
            }
          
          Show
          julianhyde Julian Hyde added a comment - Kevin Liew , I reviewed, and it looks great. Really good test coverage, nice clean code. Only cosmetic issues to fix: Can you change method javadoc comments to 3rd person declarative (e.g. prefer "Gets the label" to "Get the label"); Remove the 3 TODOs you added. Either fix the problem, or log a jira case, or remove the comment. There is one checkstyle exception. I thought that getExtendedColumns was a bit easier to understand when I added a method to transform the list of alternating names and types into a list of pairs. But it's up to you whether you take that change: public static List<RelDataTypeField> getExtendedColumns( SqlValidator validator, SqlValidatorTable table, SqlNodeList extendedColumns) { final ImmutableList.Builder<RelDataTypeField> extendedFields = ImmutableList.builder(); final ExtensibleTable extTable = table.unwrap(ExtensibleTable.class); int extendedFieldOffset = extTable == null ? table.getRowType().getFieldCount() : extTable.getExtendedColumnOffset(); for (Pair<SqlIdentifier, SqlDataTypeSpec> pair : pairs(extendedColumns)) { final SqlIdentifier identifier = pair.left; final SqlDataTypeSpec type = pair.right; extendedFields.add( new RelDataTypeFieldImpl(identifier.getSimple(), extendedFieldOffset++, type.deriveType(validator))); } return extendedFields.build(); } /** Converts a list of extended columns * (of the form [name0, type0, name1, type1, ...]) * into a list of (name, type) pairs. */ private static List<Pair<SqlIdentifier, SqlDataTypeSpec>> pairs( SqlNodeList extendedColumns) { final List list = extendedColumns.getList(); //noinspection unchecked return Pair.zip(Util.quotientList(list, 2, 0), Util.quotientList(list, 2, 1)); }
          Hide
          julianhyde Julian Hyde added a comment -

          Kevin Liew, regarding your comment

          If projectMap is a SortedMap, we could use a PriorityQueue here to improve the time complexity. wdyt?

          I think you can optimize

                    Integer projectedOrdinal = null;
                    for (Integer ordinal : modifiableViewTable.getColumnMapping()) {
                      if (entry.getKey().intValue() == ordinal.intValue()) {
                        projectedOrdinal = ordinal;
                        break;
                      }
                    }
                    if (projectedOrdinal == null) {
                      // The constrained column was not projected in the view definition.
                      continue;
                    }
          

          to something like

          if (!modifiableViewTable.getColumnMapping().contains(entry.getKey())) {
            continue;
          }
          

          And if you need to intersect the sets of column ids consider creating an ImmutableBitSet for each and using ImmutableBitSet#intersects. This compares bits in order so is basically doing a "merge".

          Show
          julianhyde Julian Hyde added a comment - Kevin Liew , regarding your comment If projectMap is a SortedMap, we could use a PriorityQueue here to improve the time complexity. wdyt? I think you can optimize Integer projectedOrdinal = null ; for ( Integer ordinal : modifiableViewTable.getColumnMapping()) { if (entry.getKey().intValue() == ordinal.intValue()) { projectedOrdinal = ordinal; break ; } } if (projectedOrdinal == null ) { // The constrained column was not projected in the view definition. continue ; } to something like if (!modifiableViewTable.getColumnMapping().contains(entry.getKey())) { continue ; } And if you need to intersect the sets of column ids consider creating an ImmutableBitSet for each and using ImmutableBitSet#intersects . This compares bits in order so is basically doing a "merge".
          Hide
          kliew Kevin Liew added a comment - - edited

          Thanks for the review Julian Hyde. I made changes based on your suggestions. I filed separate JIRA for the TODOs as they would require test cases beyond the scope of this JIRA and they aren't blocking the Phoenix-Calcite integration.

          Show
          kliew Kevin Liew added a comment - - edited Thanks for the review Julian Hyde . I made changes based on your suggestions. I filed separate JIRA for the TODOs as they would require test cases beyond the scope of this JIRA and they aren't blocking the Phoenix-Calcite integration.
          Hide
          julianhyde Julian Hyde added a comment -

          One more thing: You added quite a few imports of the rex package into SqlValidatorImpl. The validator is not supposed to depend on rex, because validation happens "before" expressions are converted to RexNode. Now, I understand that in this case the table is using RexNode to express its constraints. But still, can you think of a way to refactor this so that the RexNode dependencies don't creep into the validator? For instance, could those methods go into ModifiableViewTable? (But if so, we don't want ModifiableViewTable to depend on SqlValidator.)

          I'm going to commit your change but I'd like you to refactor the code later to get the dependencies right.

          Show
          julianhyde Julian Hyde added a comment - One more thing: You added quite a few imports of the rex package into SqlValidatorImpl. The validator is not supposed to depend on rex, because validation happens "before" expressions are converted to RexNode. Now, I understand that in this case the table is using RexNode to express its constraints. But still, can you think of a way to refactor this so that the RexNode dependencies don't creep into the validator? For instance, could those methods go into ModifiableViewTable? (But if so, we don't want ModifiableViewTable to depend on SqlValidator.) I'm going to commit your change but I'd like you to refactor the code later to get the dependencies right.
          Hide
          kliew Kevin Liew added a comment - - edited

          I am currently making some changes based on your comments in CALCITE-1680.
          I will revisit the dependency issue after giving it some thought. Thanks Julian Hyde

          Show
          kliew Kevin Liew added a comment - - edited I am currently making some changes based on your comments in CALCITE-1680 . I will revisit the dependency issue after giving it some thought. Thanks Julian Hyde
          Hide
          kliew Kevin Liew added a comment - - edited

          I ran into an issue with DDL statements (fail to validate when explicitly specifying columns of the base table in a view definition that includes extended columns).

          There doesn't seem to be an easy way to add DDL tests for Calcite as I get parser errors for create view in SqlValidatorTest. Is there an existing framework for DDL tests? I can add some testing in MockCatalogReader but that doesn't seem scalable.

          Show
          kliew Kevin Liew added a comment - - edited I ran into an issue with DDL statements (fail to validate when explicitly specifying columns of the base table in a view definition that includes extended columns). There doesn't seem to be an easy way to add DDL tests for Calcite as I get parser errors for create view in SqlValidatorTest. Is there an existing framework for DDL tests? I can add some testing in MockCatalogReader but that doesn't seem scalable.
          Hide
          kliew Kevin Liew added a comment - - edited

          Nevermind about the DDL issue - I had to specify the extended column in the select list.
          Though I have found another issue with columns reordered in the view. I will resolve the issue and test cases.

          Show
          kliew Kevin Liew added a comment - - edited Nevermind about the DDL issue - I had to specify the extended column in the select list. Though I have found another issue with columns reordered in the view. I will resolve the issue and test cases.
          Show
          julianhyde Julian Hyde added a comment - Here's a test for an extended parser that handles CREATE TABLE: https://github.com/apache/calcite/blob/e641211fd72a4f4a24f0f4644034c4b2f3c040ca/core/src/test/java/org/apache/calcite/sql/parser/parserextensiontesting/ExtensionSqlParserTest.java#L48
          Hide
          julianhyde Julian Hyde added a comment -

          In your change "Improve time complexity for INSERT to modifiable view" the code could be made less verbose and possibly more efficient. For example indexToField.get(source.getIndex()) could be changed to indexToField.containsKey(source.getIndex()), and when you notice that you're just interested in the keys in the map, you could replace the entire method with new ImmutableBitSet(source.getKeys()). Maybe you need to intersect it with ImmutableBitSet.range(sourceRowType.getFieldCount()); I'm not sure.

          Also I think getIndexToFieldMap would be more straightforward to use if it returned an immutable map, rather than populating a mutable map argument. Functional programming.

          Show
          julianhyde Julian Hyde added a comment - In your change "Improve time complexity for INSERT to modifiable view" the code could be made less verbose and possibly more efficient. For example indexToField.get(source.getIndex()) could be changed to indexToField.containsKey(source.getIndex()) , and when you notice that you're just interested in the keys in the map, you could replace the entire method with new ImmutableBitSet(source.getKeys()) . Maybe you need to intersect it with ImmutableBitSet.range(sourceRowType.getFieldCount()) ; I'm not sure. Also I think getIndexToFieldMap would be more straightforward to use if it returned an immutable map, rather than populating a mutable map argument. Functional programming.
          Hide
          julianhyde Julian Hyde added a comment -

          Please let me know when you've stopped editing this PR and I will review it again. Don't squash it; it's easier if I can see the incremental changes.

          Show
          julianhyde Julian Hyde added a comment - Please let me know when you've stopped editing this PR and I will review it again. Don't squash it; it's easier if I can see the incremental changes.
          Hide
          kliew Kevin Liew added a comment - - edited

          Thanks for the feedback Julian Hyde. The readability has improved substantially.
          The PR is ready for review again.

          For the future revision to insulate SqlValidatorImpl from Rex~ dependencies, I'd like to make SqlValidatorImpl.newValidationError a static method in SqlValidator so that it can be called from ModifiableViewTable. Does that sound like a good approach? Should I create a JIRA for the newValidationError changes?

          I will add DDL support for CREATE VIEW to the extension parser at a later date. Is that okay?

          Show
          kliew Kevin Liew added a comment - - edited Thanks for the feedback Julian Hyde . The readability has improved substantially. The PR is ready for review again. For the future revision to insulate SqlValidatorImpl from Rex~ dependencies, I'd like to make SqlValidatorImpl.newValidationError a static method in SqlValidator so that it can be called from ModifiableViewTable . Does that sound like a good approach? Should I create a JIRA for the newValidationError changes? I will add DDL support for CREATE VIEW to the extension parser at a later date. Is that okay?
          Hide
          julianhyde Julian Hyde added a comment -

          I'd like to keep newValidationError non-static. Just in case we want to use context in the validator, such as a logger.

          But I agree that it's tricky to keep the dependencies separate. How about passing in a callback (say a Function) that the ModifiableViewTable method can call on error? Or have the method populate a collection of errors?

          Show
          julianhyde Julian Hyde added a comment - I'd like to keep newValidationError non-static. Just in case we want to use context in the validator, such as a logger. But I agree that it's tricky to keep the dependencies separate. How about passing in a callback (say a Function) that the ModifiableViewTable method can call on error? Or have the method populate a collection of errors?
          Hide
          julianhyde Julian Hyde added a comment -

          Kevin Liew, The line Preconditions.checkArgument((table != null) == modifiable); fails in a few tests; yet the table field has the comment Not null if and only if the view is modifiable.. Is that comment no longer valid?

          Show
          julianhyde Julian Hyde added a comment - Kevin Liew , The line Preconditions.checkArgument((table != null) == modifiable); fails in a few tests; yet the table field has the comment Not null if and only if the view is modifiable. . Is that comment no longer valid?
          Hide
          kliew Kevin Liew added a comment - - edited

          I made some changes to satisfy the precondition check and merged the master branch.
          Working on one last change.

          Show
          kliew Kevin Liew added a comment - - edited I made some changes to satisfy the precondition check and merged the master branch. Working on one last change.
          Hide
          julianhyde Julian Hyde added a comment -

          Please, no more changes.

          Show
          julianhyde Julian Hyde added a comment - Please, no more changes.
          Hide
          kliew Kevin Liew added a comment -

          Sorry Julian, I will try to focus on productivity over perfection next time. I finalized my changes.

          Show
          kliew Kevin Liew added a comment - Sorry Julian, I will try to focus on productivity over perfection next time. I finalized my changes.
          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/e0a1f7d3 . Thanks for the PR, Kevin Liew !
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
          Hide
          kliew Kevin Liew added a comment -

          I refactored out some Rex~ dependencies in https://github.com/apache/calcite/pull/419

          Show
          kliew Kevin Liew added a comment - I refactored out some Rex~ dependencies in https://github.com/apache/calcite/pull/419
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Thanks Kevin Liew , I've committed that refactoring as http://git-wip-us.apache.org/repos/asf/calcite/commit/059142fe .

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              kliew Kevin Liew
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development