Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.12.0
    • Fix Version/s: 1.13.0
    • Component/s: None
    • Labels:
      None

      Description

      DML support was partially implemented in CALCITE-493.
      I am working on a patch for the implementation and tests.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Regarding ModifiableViewTable.typeFactory. It's not OK to store a type factory, or a type, in a table. (Type factory has the life cycle of a statement, but a table can live longer; if we hold types in tables, we will get memory leaks.) Remember that a RelProtoDataType is just a lambda - a "just add water" - something that becomes a type when you bind to a type factory at a later date. So, you can push all of that logic in ModifiableViewTable.extend into the lambda.

          Can you add a few SqlParserTest tests for extended DML.

          Show
          julianhyde Julian Hyde added a comment - Regarding ModifiableViewTable.typeFactory . It's not OK to store a type factory, or a type, in a table. (Type factory has the life cycle of a statement, but a table can live longer; if we hold types in tables, we will get memory leaks.) Remember that a RelProtoDataType is just a lambda - a "just add water" - something that becomes a type when you bind to a type factory at a later date. So, you can push all of that logic in ModifiableViewTable.extend into the lambda. Can you add a few SqlParserTest tests for extended DML.
          Hide
          kliew Kevin Liew added a comment -

          Thanks Julian. I found a similar problem with ModifiableViewTable storing ModifiableViewTableInitializerExpressionFactory which extends NullInitializerExpressionFactory and stores RexBuilder.typeFactory. I modified the interface for InitializerExpressionFactory to use a type factory per statement since InitializerExpressionFactory is typically stored in a Table. The PR has been updated with these changes.

          I will add more test cases in SqlParserTest, test cases for case sensitivity on extended columns, and for handling the case where an extended column (of a table, and of a view) has the same name but differing data-type from a column of the base table as discussed in: https://lists.apache.org/thread.html/a2a882b327368d052d074eeb433be794f8d58a31c18fe05a32fae43d@<dev.calcite.apache.org>

          Support for qualified column names (nested columns or column families) for extended columns will likely be deferred to a second JIRA as it looks to involve more changes to the parser and I don't want to create a monolithic pull request.

          Show
          kliew Kevin Liew added a comment - Thanks Julian. I found a similar problem with ModifiableViewTable storing ModifiableViewTableInitializerExpressionFactory which extends NullInitializerExpressionFactory and stores RexBuilder.typeFactory . I modified the interface for InitializerExpressionFactory to use a type factory per statement since InitializerExpressionFactory is typically stored in a Table . The PR has been updated with these changes. I will add more test cases in SqlParserTest, test cases for case sensitivity on extended columns, and for handling the case where an extended column (of a table, and of a view) has the same name but differing data-type from a column of the base table as discussed in: https://lists.apache.org/thread.html/a2a882b327368d052d074eeb433be794f8d58a31c18fe05a32fae43d@ <dev.calcite.apache.org> Support for qualified column names (nested columns or column families) for extended columns will likely be deferred to a second JIRA as it looks to involve more changes to the parser and I don't want to create a monolithic pull request.
          Hide
          julianhyde Julian Hyde added a comment -

          Agreed, let's do several PRs, not one monolithic one. Only a small number of parser tests are needed. When you have something ready, please rebase and squash and update this JIRA case.

          Show
          julianhyde Julian Hyde added a comment - Agreed, let's do several PRs, not one monolithic one. Only a small number of parser tests are needed. When you have something ready, please rebase and squash and update this JIRA case.
          Hide
          kliew Kevin Liew added a comment -

          The PR is ready for review. Support for column qualifiers is deferred to CALCITE-1728.

          Show
          kliew Kevin Liew added a comment - The PR is ready for review. Support for column qualifiers is deferred to CALCITE-1728 .
          Hide
          julianhyde Julian Hyde added a comment -

          Looks good. I have fixed up a bit, moving class RexBuilderHolder to a interface InitializerContext, and removed the RelDataTypeFactory arguments of TableNamespace.extend(SqlNodeList, RelDataTypeFactory) and RelOptTable.extend(List<RelDataTypeField>, RelDataTypeFactory). I've pushed to https://github.com/julianhyde/calcite/commits/master and will shortly push to Apache.

          Show
          julianhyde Julian Hyde added a comment - Looks good. I have fixed up a bit, moving class RexBuilderHolder to a interface InitializerContext , and removed the RelDataTypeFactory arguments of TableNamespace.extend(SqlNodeList, RelDataTypeFactory) and RelOptTable.extend(List<RelDataTypeField>, RelDataTypeFactory) . I've pushed to https://github.com/julianhyde/calcite/commits/master and will shortly push to Apache.
          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/8a1a287d . Thanks for the PR, Kevin Liew !
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.13.0 (2017-06-26).

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.13.0 (2017-06-26).

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development