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

Support mixing table columns with extended columns in DML

    Details

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

      Description

      Phoenix allows embedding dynamic column definitions in the UPDATE target column list whereas Calcite requires that extended columns be specified separately from the target columns.

      ie.
      Phoenix

      UPSERT INTO EventLog
      (eventId, eventTime, eventType, lastGCTime TIME, usedMemory BIGINT, maxMemory BIGINT)
      VALUES (1, CURRENT_TIME(), ‘abc’, CURRENT_TIME(), 512, 1024);
      

      Calcite

      UPSERT INTO EventLog
      (lastGCTime TIME, usedMemory BIGINT, maxMemory BIGINT)
      (eventId, eventTime, eventType, lastGCTime, usedMemory, maxMemory)
      VALUES (1, CURRENT_TIME(), ‘abc’, CURRENT_TIME(), 512, 1024);
      

      We should have a conformance setting for this feature. https://issues.apache.org/jira/browse/PHOENIX-3732?focusedCommentId=15930704&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15930704

        Issue Links

          Activity

          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)
          Hide
          julianhyde Julian Hyde added a comment -

          Rajeshbabu Chintaguntla logged CALCITE-1889 as the follow-up case.

          Show
          julianhyde Julian Hyde added a comment - Rajeshbabu Chintaguntla logged CALCITE-1889 as the follow-up case.
          Hide
          julianhyde Julian Hyde added a comment -

          As I said, we need a new JIRA case, and only when we think we've found all the issues.

          Comments:

          • In SqlValidatorTest, sql0 should be final.
          • Can you move the new relation into MockCatalogReader.init2, and use withExtendedCatalog2003 just as testInsertSubsetModifiableView does. We're trying to keep the mock catalog as simple as possible.
          • In testCheckingDuplicatesWithCompoundIdentifiers there's no need to catch AssertionError. And can you add a negative test that is supposed to fail and throw CalciteContextException.
          Show
          julianhyde Julian Hyde added a comment - As I said, we need a new JIRA case, and only when we think we've found all the issues. Comments: In SqlValidatorTest, sql0 should be final. Can you move the new relation into MockCatalogReader.init2, and use withExtendedCatalog2003 just as testInsertSubsetModifiableView does. We're trying to keep the mock catalog as simple as possible. In testCheckingDuplicatesWithCompoundIdentifiers there's no need to catch AssertionError. And can you add a negative test that is supposed to fail and throw CalciteContextException.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde
          I made changes to PR https://github.com/apache/calcite/pull/487 to compare duplicates component by component.
          Please review.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde I made changes to PR https://github.com/apache/calcite/pull/487 to compare duplicates component by component. Please review.
          Hide
          julianhyde Julian Hyde added a comment -

          I don't like the change from getSimple to toString. I don't want compound identifiers to become "normal".

          Can you please open a new JIRA case for this? Let's leave it open for a while, and I will review & commit when we are pretty sure we have found all issues.

          Show
          julianhyde Julian Hyde added a comment - I don't like the change from getSimple to toString. I don't want compound identifiers to become "normal". Can you please open a new JIRA case for this? Let's leave it open for a while, and I will review & commit when we are pretty sure we have found all issues.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde SqlValidatorUtil#checkIdentifierListForDuplicates always expecting simple identifiers but we are supporting compound identifiers also in the extend so it may throw AssertionError. Got it when I ran some Phoenix test with assertions enabled otherwise the tests were passing.
          Created https://github.com/apache/calcite/pull/487 for handling the same and added some test cases related to this issue. Can you please review.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde SqlValidatorUtil#checkIdentifierListForDuplicates always expecting simple identifiers but we are supporting compound identifiers also in the extend so it may throw AssertionError. Got it when I ran some Phoenix test with assertions enabled otherwise the tests were passing. Created https://github.com/apache/calcite/pull/487 for handling the same and added some test cases related to this issue. Can you please review.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Thanks Julian Hyde,Jinfeng Ni for review.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Thanks Julian Hyde , Jinfeng Ni for review.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a6fbafde . Thanks for the PR, Rajeshbabu Chintaguntla .
          Hide
          julianhyde Julian Hyde added a comment -

          Jinfeng Ni, Thanks.

          Show
          julianhyde Julian Hyde added a comment - Jinfeng Ni , Thanks.
          Hide
          jni Jinfeng Ni added a comment -

          I looked the change in Parser.jj and seems to me the syntax for CompoundIdentifier() remains same as before. It does not look like the change would impact Drill.

          Show
          jni Jinfeng Ni added a comment - I looked the change in Parser.jj and seems to me the syntax for CompoundIdentifier() remains same as before. It does not look like the change would impact Drill.
          Hide
          jni Jinfeng Ni added a comment -

          Julian Hyde, thanks for the reminder. I'll take a look.

          Show
          jni Jinfeng Ni added a comment - Julian Hyde , thanks for the reminder. I'll take a look.
          Hide
          julianhyde Julian Hyde added a comment -

          Jinfeng Ni, The changes to ParenthesizedCompoundIdentifierList in Parser.jj might affect Drill, because CALCITE-321 was for Drill's benefit. Can you review please?

          Show
          julianhyde Julian Hyde added a comment - Jinfeng Ni , The changes to ParenthesizedCompoundIdentifierList in Parser.jj might affect Drill, because CALCITE-321 was for Drill's benefit. Can you review please?
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde Updated pull request by handling review comments. Added spaces after commas, spelling mistake and added test case in SqlParserTest. Please review. Thanks.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde Updated pull request by handling review comments. Added spaces after commas, spelling mistake and added test case in SqlParserTest. Please review. Thanks.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Thank you very much for review Julian Hyde.

          Need spaces after commas in both code and comments, s/optinal/optional

          Will change it.

          Using '.' in field names is a hack; log a JIRA case describing what is broken by this hack

          Raised CALCITE-1858 for supporting compound identifiers as extended fields.

          I know you added a validator test, but can you also add a parser test. See SqlParserTest.testTableExtend.

          Sure will add the test for this.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Thank you very much for review Julian Hyde . Need spaces after commas in both code and comments, s/optinal/optional Will change it. Using '.' in field names is a hack; log a JIRA case describing what is broken by this hack Raised CALCITE-1858 for supporting compound identifiers as extended fields. I know you added a validator test, but can you also add a parser test. See SqlParserTest.testTableExtend. Sure will add the test for this.
          Hide
          julianhyde Julian Hyde added a comment -

          Review comments:

          • Need spaces after commas in both code and comments
          • s/optinal/optional
          • Using '.' in field names is a hack; log a JIRA case describing what is broken by this hack
          • I know you added a validator test, but can you also add a parser test. See SqlParserTest.testTableExtend.
          Show
          julianhyde Julian Hyde added a comment - Review comments: Need spaces after commas in both code and comments s/optinal/optional Using '.' in field names is a hack; log a JIRA case describing what is broken by this hack I know you added a validator test, but can you also add a parser test. See SqlParserTest.testTableExtend.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Julian Hyde https://github.com/apache/calcite/pull/482 is pr created for this. Please review.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Julian Hyde https://github.com/apache/calcite/pull/482 is pr created for this. Please review.
          Hide
          rajeshbabu Rajeshbabu Chintaguntla added a comment -

          Will take up this as Kevin Liew on vacation.

          Show
          rajeshbabu Rajeshbabu Chintaguntla added a comment - Will take up this as Kevin Liew on vacation.

            People

            • Assignee:
              rajeshbabu Rajeshbabu Chintaguntla
              Reporter:
              kliew Kevin Liew
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development