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

Cannot assign NOT NULL array to NULLABLE array

    Details

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

      Description

      As ArraySqlType return a family of its own type and comparing families in SqlTypeUtil#canAssignFrom will compare the digest with Nullable constraints, which will not match when we are inserting an array in a nullable column.

        Issue Links

          Activity

          Hide
          ankit@apache.org Ankit Singhal added a comment -
          Show
          ankit@apache.org Ankit Singhal added a comment - I have tried something here. https://github.com/apache/calcite/pull/456/files
          Hide
          julianhyde Julian Hyde added a comment -

          Can you provide a test case?

          Show
          julianhyde Julian Hyde added a comment - Can you provide a test case?
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          Julian Hyde, just added a test case on pull request.
          And, from where I can download the formatter and import order related configs for eclipse.

          Show
          ankit@apache.org Ankit Singhal added a comment - Julian Hyde , just added a test case on pull request. And, from where I can download the formatter and import order related configs for eclipse.
          Hide
          julianhyde Julian Hyde added a comment -

          You don't need a formatter - you can format manually. Just make sure your editor doesn't make spurious changes.

          How about making canAssignFrom call itself recursively on the component type? Your code won't handle assigning an array of arrays of not null integers to an array of arrays of nullable integers, but the recursive solution would, and would be simpler.

          Please make sure that the tests and validate/verify pass before submitting the PR.

          Show
          julianhyde Julian Hyde added a comment - You don't need a formatter - you can format manually. Just make sure your editor doesn't make spurious changes. How about making canAssignFrom call itself recursively on the component type? Your code won't handle assigning an array of arrays of not null integers to an array of arrays of nullable integers, but the recursive solution would, and would be simpler. Please make sure that the tests and validate/verify pass before submitting the PR.
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing now.

          Show
          julianhyde Julian Hyde added a comment - Reviewing now.
          Hide
          julianhyde Julian Hyde added a comment -
          • Why do you need a change to JavaToSqlTypeConversionRules? And why org.apache.calcite.avatica.util.ArrayImpl and not java.sql.Array?
          • Why the change to JavaTypeFactoryImpl? I can see it might make sense, but it's not justified by any test case.
          Show
          julianhyde Julian Hyde added a comment - Why do you need a change to JavaToSqlTypeConversionRules? And why org.apache.calcite.avatica.util.ArrayImpl and not java.sql.Array? Why the change to JavaTypeFactoryImpl? I can see it might make sense, but it's not justified by any test case.
          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/f164fb8e . Thanks for the PR, Ankit Singhal !
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          Thanks Julian Hyde for committing this.

          Why do you need a change to JavaToSqlTypeConversionRules? And why org.apache.calcite.avatica.util.ArrayImpl and not java.sql.Array?

          I thought ArrayImpl represents java.sql.Array in calcite/avatica. so started declaring Phoenix array data types with Java Type as ArrayImpl and added a conversion rule for the same. But not sure if it can be java.sql.Array or java.util.List or anything.

          Why the change to JavaTypeFactoryImpl? I can see it might make sense, but it's not justified by any test case.

          Array under basicSqlType seems to me was just an oversight. Actually discovered when Phoenix-calcite integration originally was creating Array as basicSqlType.(but has not impact earlier too)

          Show
          ankit@apache.org Ankit Singhal added a comment - Thanks Julian Hyde for committing this. Why do you need a change to JavaToSqlTypeConversionRules? And why org.apache.calcite.avatica.util.ArrayImpl and not java.sql.Array? I thought ArrayImpl represents java.sql.Array in calcite/avatica. so started declaring Phoenix array data types with Java Type as ArrayImpl and added a conversion rule for the same. But not sure if it can be java.sql.Array or java.util.List or anything. Why the change to JavaTypeFactoryImpl? I can see it might make sense, but it's not justified by any test case. Array under basicSqlType seems to me was just an oversight. Actually discovered when Phoenix-calcite integration originally was creating Array as basicSqlType.(but has not impact earlier too)
          Hide
          ankit@apache.org Ankit Singhal added a comment -

          And is it possible to commit CALCITE-1804.patch too under the same Jira.(as addendum)

          Show
          ankit@apache.org Ankit Singhal added a comment - And is it possible to commit CALCITE-1804.patch too under the same Jira.(as addendum)
          Hide
          julianhyde Julian Hyde added a comment -

          You need a test case. Without a test case I don't know what problem your code change is supposed to solve.

          Also please make it work for MULTISET, which I assume is similar.

          When you have a description of the problem you are solving, you might as well create a JIRA case with a pull request.

          Show
          julianhyde Julian Hyde added a comment - You need a test case. Without a test case I don't know what problem your code change is supposed to solve. Also please make it work for MULTISET, which I assume is similar. When you have a description of the problem you are solving, you might as well create a JIRA case with a pull request.
          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:
              ankit@apache.org Ankit Singhal
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development