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

INSERT/UPSERT should allow fewer values than columns

    Details

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

      Description

      Currently calcite not allowing to write if the values less than number of columns. We can write the leading columns with the all values and remaining columns can be considered as null or else we can allow specifying columns and corresponding values. Ping Julian Hyde James Taylor Maryann Xue.

      1. CALCITE-1510_addendum.patch
        1 kB
        Rajeshbabu Chintaguntla
      2. CALCITE-1510.2.patch
        52 kB
        Kevin Liew
      3. CALCITE-1510.3.patch
        83 kB
        Kevin Liew
      4. CALCITE-1510.4.patch
        80 kB
        Kevin Liew
      5. CALCITE-1510.5.patch
        82 kB
        Kevin Liew
      6. CALCITE-1510.patch
        2 kB
        Kevin Liew

        Issue Links

          Activity

          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
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Addendum committed as http://git-wip-us.apache.org/repos/asf/calcite/commit/2218343f .
          Hide
          julianhyde Julian Hyde added a comment -

          Thanks - I will review.

          Show
          julianhyde Julian Hyde added a comment - Thanks - I will review.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde Here is the pull request created for the same with test case. Please review.
          https://github.com/apache/calcite/pull/383

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde Here is the pull request created for the same with test case. Please review. https://github.com/apache/calcite/pull/383
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde sure will add test if possible and create pull request.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde sure will add test if possible and create pull request.
          Hide
          julianhyde Julian Hyde added a comment -

          Rajeshbabu Chintaguntla, Looks good, but can you please add a test case and submit as a pull request not a patch?

          Show
          julianhyde Julian Hyde added a comment - Rajeshbabu Chintaguntla , Looks good, but can you please add a test case and submit as a pull request not a patch?
          Hide
          kliew Kevin Liew added a comment -
          Show
          kliew Kevin Liew added a comment - +1, thanks Rajeshbabu Chintaguntla
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde Kevin Liew
          In the below code we can check for default value only when the field in null in target list.

                  final boolean haveDefaultValue =
                      table.columnHasDefaultValue(table.getRowType(), field.getIndex());
                  if (targetField == null && !haveDefaultValue) {
                    throw newValidationError(node,
                        RESOURCE.columnNotNullable(field.getName()));
          

          Here is the addendum patch for the same.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde Kevin Liew In the below code we can check for default value only when the field in null in target list. final boolean haveDefaultValue = table.columnHasDefaultValue(table.getRowType(), field.getIndex()); if (targetField == null && !haveDefaultValue) { throw newValidationError(node, RESOURCE.columnNotNullable(field.getName())); Here is the addendum patch for the same.
          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/08b1dddd . Thanks for the patches, Kevin Liew !
          Hide
          kliew Kevin Liew added a comment - - edited

          This last patch is final, unless you have any suggestions after reviewing the patch.

          Show
          kliew Kevin Liew added a comment - - edited This last patch is final, unless you have any suggestions after reviewing the patch.
          Hide
          julianhyde Julian Hyde added a comment -

          Please let me know when you are done and I will review your final patch.

          Show
          julianhyde Julian Hyde added a comment - Please let me know when you are done and I will review your final patch.
          Hide
          kliew Kevin Liew added a comment -

          Julian Hyde, I attached a patch (with the appropriate author tags) for review. SqlValidatorTable can now unwrap Table in SqlToRelConverter. Let me know your feedback on this. All test cases are passing except one which failed in the master branch as well.

          Show
          kliew Kevin Liew added a comment - Julian Hyde , I attached a patch (with the appropriate author tags) for review. SqlValidatorTable can now unwrap Table in SqlToRelConverter . Let me know your feedback on this. All test cases are passing except one which failed in the master branch as well.
          Hide
          kliew Kevin Liew added a comment -

          Sorry it's a bit late for a review so this next patch is more of a progress update.
          I added support and test coverage for Table unwrapping InitializerExpressionFactory. I will continue to work on SqlToRelConverter changes - I'd like to be able to unwrap Table from SqlValidatorTable.

          Show
          kliew Kevin Liew added a comment - Sorry it's a bit late for a review so this next patch is more of a progress update. I added support and test coverage for Table unwrapping InitializerExpressionFactory . I will continue to work on SqlToRelConverter changes - I'd like to be able to unwrap Table from SqlValidatorTable .
          Hide
          kliew Kevin Liew added a comment -

          I found WrapperTable that can be used for testing InitializerExpressionFactory.
          I'm still searching for a better way to get InitializerExpressionFactory in SqlToRelConverter but we may have to pass it as a parameter or through the validator.

          Show
          kliew Kevin Liew added a comment - I found WrapperTable that can be used for testing InitializerExpressionFactory . I'm still searching for a better way to get InitializerExpressionFactory in SqlToRelConverter but we may have to pass it as a parameter or through the validator.
          Hide
          julianhyde Julian Hyde added a comment -

          I'll take a look.

          Show
          julianhyde Julian Hyde added a comment - I'll take a look.
          Hide
          kliew Kevin Liew added a comment -

          Thank you for the feedback Julian Hyde. I attached a new patch addressing these issues.

          SqlValidatorImpl now uses SqlValidatorTable.columnHasDefaultValue rather than using InitializerExpressionFactory directly.
          The validator still owns an InitializerExpressionFactory to pass the SqlToRelConverter.

          The new patch does comparison of NULL by comparing the SqlTypeName and also validates inserting NULL against column nullability.

          I added more test coverage and I am working on test cases for a custom Table wrapping a custom InitializerExpressionFactory to verify that it can work for Phoenix.

          I plan on having AbstractPreparingTable own a Table so that MockTable will handle test cases for custom InitializerExpressionFactory. Also, this might allow SqlToRelConverter to unwrap the InitializerExpressionFactory instead of passing it through the validator.

          Show
          kliew Kevin Liew added a comment - Thank you for the feedback Julian Hyde . I attached a new patch addressing these issues. SqlValidatorImpl now uses SqlValidatorTable.columnHasDefaultValue rather than using InitializerExpressionFactory directly. The validator still owns an InitializerExpressionFactory to pass the SqlToRelConverter . The new patch does comparison of NULL by comparing the SqlTypeName and also validates inserting NULL against column nullability. I added more test coverage and I am working on test cases for a custom Table wrapping a custom InitializerExpressionFactory to verify that it can work for Phoenix. I plan on having AbstractPreparingTable own a Table so that MockTable will handle test cases for custom InitializerExpressionFactory . Also, this might allow SqlToRelConverter to unwrap the InitializerExpressionFactory instead of passing it through the validator.
          Hide
          julianhyde Julian Hyde added a comment -

          If there is an explicit target column list, Calcite already accepts fewer columns than the number of columns in the table, and that behavior should not change. This bug is only about the behavior WITHOUT an explicit target column list. For example, in testInsertBind:

          final String sql2 = "insert into empnullables (ename, empno)\n"
                  + "select ?, ? from (values (1))\n"
                  + "union all\n"
                  + "select ?, ? from (values (time '1:2:3'))";
          final String expected2 = "RecordType(VARCHAR(20) ?0, INTEGER ?1,"
                   + " VARCHAR(20) ?2, INTEGER ?3)";
             sql(sql2).tester(pragmaticTester).ok().bindType(expected2);
          

          should work even if you remove the .tester(pragmaticTester).

          Show
          julianhyde Julian Hyde added a comment - If there is an explicit target column list, Calcite already accepts fewer columns than the number of columns in the table, and that behavior should not change. This bug is only about the behavior WITHOUT an explicit target column list. For example, in testInsertBind : final String sql2 = "insert into empnullables (ename, empno)\n" + "select ?, ? from (values (1))\n" + "union all\n" + "select ?, ? from (values (time '1:2:3'))" ; final String expected2 = "RecordType(VARCHAR(20) ?0, INTEGER ?1," + " VARCHAR(20) ?2, INTEGER ?3)" ; sql(sql2).tester(pragmaticTester).ok().bindType(expected2); should work even if you remove the .tester(pragmaticTester) .
          Hide
          julianhyde Julian Hyde added a comment -

          One more:

          • Now might be a good time to rename DefaultValueFactory to InitializerExpressionFactory
          Show
          julianhyde Julian Hyde added a comment - One more: Now might be a good time to rename DefaultValueFactory to InitializerExpressionFactory
          Hide
          julianhyde Julian Hyde added a comment -

          I've made a fix-up commit in https://github.com/julianhyde/calcite/tree/1510-short-insert (minor refactorings, mainly). Please work off of that branch.

          A few thoughts:

          • I am wondering whether a DefaultValueFactory belongs in SqlValidator; note that it depends on RexNode, whereas the validator works exclusively in terms of SqlNode. (DefaultValueFactory is fine in SqlToRelConverter.)
          • Maybe add boolean columnHasDefaultValue(RelDataType rowType, int ordinal) to SqlValidatorTable. The default implementation would just look whether the column allows nulls, but RelOptTableImpl could do something more sophisticated, maybe calling table.unwrap(DefaultValueFactory.class)
          • Would wrapping DefaultValueFactory inside the Table work in Phoenix's environment?
          • I don't like how you compare the value of the defaultValueFactory with the value from the nullDefaultValueFactory. There must be a less convoluted (and less brittle) way to achieve this.
          • The message "Column 'x' is not nullable" should be "Column 'x' has no default value and does not allow NULLs"
          • Maybe I missed them, but are there tests that do the same queries as testInsertSubset but use the default conformance (and therefore give errors)?
          • See the 'TODO' I added
          Show
          julianhyde Julian Hyde added a comment - I've made a fix-up commit in https://github.com/julianhyde/calcite/tree/1510-short-insert (minor refactorings, mainly). Please work off of that branch. A few thoughts: I am wondering whether a DefaultValueFactory belongs in SqlValidator ; note that it depends on RexNode , whereas the validator works exclusively in terms of SqlNode . ( DefaultValueFactory is fine in SqlToRelConverter .) Maybe add boolean columnHasDefaultValue(RelDataType rowType, int ordinal) to SqlValidatorTable . The default implementation would just look whether the column allows nulls, but RelOptTableImpl could do something more sophisticated, maybe calling table.unwrap(DefaultValueFactory.class) Would wrapping DefaultValueFactory inside the Table work in Phoenix's environment? I don't like how you compare the value of the defaultValueFactory with the value from the nullDefaultValueFactory. There must be a less convoluted (and less brittle) way to achieve this. The message "Column 'x' is not nullable" should be "Column 'x' has no default value and does not allow NULLs" Maybe I missed them, but are there tests that do the same queries as testInsertSubset but use the default conformance (and therefore give errors)? See the 'TODO' I added
          Hide
          kliew Kevin Liew added a comment -

          Julian Hyde, some test cases are currently failing due to differences in the explain plan since the expected explain plans have not yet been coded into the result set files.
          I'd appreciate any pointers on the explain-result framework.
          I will look into this tonight and create a pull request when all test cases are passing.

          Show
          kliew Kevin Liew added a comment - Julian Hyde , some test cases are currently failing due to differences in the explain plan since the expected explain plans have not yet been coded into the result set files. I'd appreciate any pointers on the explain-result framework. I will look into this tonight and create a pull request when all test cases are passing.
          Hide
          julianhyde Julian Hyde added a comment -

          Kevin Liew, What is your email address? I want to set the git author so that you get credit for the changes. (Or you could submit a pull request.)

          Show
          julianhyde Julian Hyde added a comment - Kevin Liew , What is your email address? I want to set the git author so that you get credit for the changes. (Or you could submit a pull request.)
          Hide
          kliew Kevin Liew added a comment - - edited

          I attached another patch to the master branch with validation using DefaultValueFactory.

          The prepare context delegates retrieval of the DefaultValueFactory to CalciteConnectionImpl. This way the datasource only needs to override CalciteConnectionImpl.getDefaultValueFactory.

          Otherwise the datasource would need to extend the prepare context as well as CalciteStatement and CalciteServerStatement as they both have createPrepareContext calls which would need to return the extended context - so I didn't use that approach.

          I'm still working on adding test cases. I also performed integration tests in Phoenix using the patch at PHOENIX-3579 against Calcite 1.11. Let me know your feedback Maryann Xue, Rajeshbabu Chintaguntla, Julian Hyde.

          Show
          kliew Kevin Liew added a comment - - edited I attached another patch to the master branch with validation using DefaultValueFactory. The prepare context delegates retrieval of the DefaultValueFactory to CalciteConnectionImpl. This way the datasource only needs to override CalciteConnectionImpl.getDefaultValueFactory . Otherwise the datasource would need to extend the prepare context as well as CalciteStatement and CalciteServerStatement as they both have createPrepareContext calls which would need to return the extended context - so I didn't use that approach. I'm still working on adding test cases. I also performed integration tests in Phoenix using the patch at PHOENIX-3579 against Calcite 1.11. Let me know your feedback Maryann Xue , Rajeshbabu Chintaguntla , Julian Hyde .
          Hide
          julianhyde Julian Hyde added a comment -

          It doesn't seem right to add it to RelDataType. Note that we already have DefaultValueFactory and SqlToRelConverter uses it.

          Show
          julianhyde Julian Hyde added a comment - It doesn't seem right to add it to RelDataType . Note that we already have DefaultValueFactory and SqlToRelConverter uses it.
          Hide
          kliew Kevin Liew added a comment - - edited

          Thanks Julian Hyde. I now have a good handle on what needs to be done. After some discussion it was decided to leave materialization of values to the datastore, but validation still needs to be done based on nullability and the existence of values - I'd like to add a method to RelDataType that indicates whether there exists a default value.
          ie. Phoenix can use SqlToRelConverter with the default NullDefaultValueFactory . In PhoenixTableModify, the source fields with default values will have null values. When executing the physical plan, Phoenix will insert null byte-arrays indicating default values (Phoenix-specific optimization) based on RelDataType.hasDefaultValue

          Show
          kliew Kevin Liew added a comment - - edited Thanks Julian Hyde . I now have a good handle on what needs to be done. After some discussion it was decided to leave materialization of values to the datastore, but validation still needs to be done based on nullability and the existence of values - I'd like to add a method to RelDataType that indicates whether there exists a default value. ie. Phoenix can use SqlToRelConverter with the default NullDefaultValueFactory . In PhoenixTableModify, the source fields with default values will have null values. When executing the physical plan, Phoenix will insert null byte-arrays indicating default values (Phoenix-specific optimization) based on RelDataType.hasDefaultValue
          Hide
          julianhyde Julian Hyde added a comment - - edited

          I don't think we should weaken the relational algebra (Values or TableModificationRel). If the table has N columns then the relation going into it must have N columns. It is to everyone's benefit to keep the relational algebra very strict.

          You should change the validator to allow fewer columns if (a) the conformance enables it and (b) the omitted columns are nullable or have default values. Then change the sql-to-rel converter to supply the default values: so INSERT INTO t (x) VALUES (1) has a very similar effect to INSERT INTO t (x, y, z) VALUES (1, DEFAULT, DEFAULT).

          And we will need tests (mostly in SqlValidatorTest).

          Show
          julianhyde Julian Hyde added a comment - - edited I don't think we should weaken the relational algebra ( Values or TableModificationRel ). If the table has N columns then the relation going into it must have N columns. It is to everyone's benefit to keep the relational algebra very strict. You should change the validator to allow fewer columns if (a) the conformance enables it and (b) the omitted columns are nullable or have default values. Then change the sql-to-rel converter to supply the default values: so INSERT INTO t (x) VALUES (1) has a very similar effect to INSERT INTO t (x, y, z) VALUES (1, DEFAULT, DEFAULT) . And we will need tests (mostly in SqlValidatorTest).
          Hide
          kliew Kevin Liew added a comment - - edited

          Though I realize now that I should make the conformance level configurable, update the error message accordingly, and familiarize myself with the Calcite testing infrastructure.

          Show
          kliew Kevin Liew added a comment - - edited Though I realize now that I should make the conformance level configurable, update the error message accordingly, and familiarize myself with the Calcite testing infrastructure.
          Hide
          kliew Kevin Liew added a comment -

          Thanks Julian Hyde, I attached a patch. I believe most of the changes will be in PHOENIX-3579.

          Show
          kliew Kevin Liew added a comment - Thanks Julian Hyde , I attached a patch. I believe most of the changes will be in PHOENIX-3579 .
          Hide
          julianhyde Julian Hyde added a comment -

          Kevin Liew, Here's the case I mentioned.

          Show
          julianhyde Julian Hyde added a comment - Kevin Liew , Here's the case I mentioned.
          Hide
          julianhyde Julian Hyde added a comment -

          I believe (can someone please confirm/deny), that on Oracle, if I say insert into emp values (100, 20, 'Fred') and emp has 4 columns empno int, deptno int, name varchar(20), comm number, Oracle will give an error. If so, we will need a new conformance method to allow this.

          Now, I believe that you can say insert into emp (empno, deptno, name) values (100, 20, 'Fred'), but that is not what we are talking about here.

          Show
          julianhyde Julian Hyde added a comment - I believe (can someone please confirm/deny), that on Oracle, if I say insert into emp values (100, 20, 'Fred') and emp has 4 columns empno int, deptno int, name varchar(20), comm number , Oracle will give an error. If so, we will need a new conformance method to allow this. Now, I believe that you can say insert into emp (empno, deptno, name) values (100, 20, 'Fred') , but that is not what we are talking about here.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              rajeshbabu Rajeshbabu Chintaguntla
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development