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

Error while validating UPDATE with dynamic parameter in SET clause

    Details

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

      Description

      with a simple UPDATE like:
      UPDATE mytable set a=? where b=1

      I get the error below.
      The "Table" is a ModifiableTable + ScannableTable, with "a" of type INTEGER and "b" of type VARCHAR

      Any hint ?
      Thank you
      Enrico

      org.apache.calcite.runtime.CalciteContextException: At line 1, column 30: Illegal use of dynamic parameter
          at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
          at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
          at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
          at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
          at org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:463)
          at org.apache.calcite.sql.SqlUtil.newContextException(SqlUtil.java:803)
          at org.apache.calcite.sql.SqlUtil.newContextException(SqlUtil.java:788)
          at org.apache.calcite.sql.validate.SqlValidatorImpl.newValidationError(SqlValidatorImpl.java:4651)
          at org.apache.calcite.sql.validate.SqlValidatorImpl.inferUnknownTypes(SqlValidatorImpl.java:1694)
          at org.apache.calcite.sql.validate.SqlValidatorImpl.inferUnknownTypes(SqlValidatorImpl.java:1769)
          at org.apache.calcite.sql.validate.SqlValidatorImpl.expandSelectItem(SqlValidatorImpl.java:457)
          at org.apache.calcite.sql.validate.SqlValidatorImpl.expandStar(SqlValidatorImpl.java:347)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectList(SqlToRelConverter.java:3709)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectImpl(SqlToRelConverter.java:663)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelect(SqlToRelConverter.java:620)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertUpdate(SqlToRelConverter.java:3398)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertQueryRecursive(SqlToRelConverter.java:3048)
          at org.apache.calcite.sql2rel.SqlToRelConverter.convertQuery(SqlToRelConverter.java:556)
          at org.apache.calcite.prepare.PlannerImpl.rel(PlannerImpl.java:240)
      

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.15.0 (2017-12-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
          Hide
          eolivelli Enrico Olivelli added a comment -

          Thank you so much!

          Show
          eolivelli Enrico Olivelli added a comment - Thank you so much!
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in cd493efd; thanks for the PR, Enrico Olivelli!

          Show
          julianhyde Julian Hyde added a comment - Fixed in cd493efd ; thanks for the PR, Enrico Olivelli !
          Hide
          eolivelli Enrico Olivelli added a comment -

          Julian Hyde I have added the test as requested.

          Show
          eolivelli Enrico Olivelli added a comment - Julian Hyde I have added the test as requested.
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing https://github.com/apache/calcite/pull/568/commits/d3d56a8ae67ea5af2b6dd5371f5b364938bab2b9:

          • Looks better.
          • It doesn't seem possible to execute UPDATE in tests (see below), but can you add a test where you call connection.prepareStatement().getParameterMetaData() and make sure that the bind variable has the right type?

          Here is the update test I tried to add; it gave an error (not your fault, and we should fix it sometime):

          diff --git a/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java b/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java
          index da6facc..898e89e 100644
          --- a/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java
          +++ b/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java
          @@ -294,6 +294,35 @@ public Object apply(CalciteConnection c) {
                       "name=first; C=1");
             }
           
          +  @Test public void testUpdate() {
          +    final List<JdbcTest.Employee> employees = new ArrayList<>();
          +    CalciteAssert.AssertThat with = mutable(employees);
          +    with.query("select * from \"foo\".\"bar\"")
          +        .returnsUnordered(
          +            "empid=0; deptno=0; name=first; salary=0.0; commission=null");
          +    with.query("insert into \"foo\".\"bar\" select * from \"hr\".\"emps\"")
          +        .updates(4);
          +    with.query("select * from \"foo\".\"bar\"")
          +        .returnsUnordered(
          +            "empid=0; deptno=0; name=first; salary=0.0; commission=null",
          +            "empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000",
          +            "empid=110; deptno=10; name=Theodore; salary=11500.0; commission=250",
          +            "empid=150; deptno=10; name=Sebastian; salary=7000.0; commission=null",
          +            "empid=200; deptno=20; name=Eric; salary=8000.0; commission=500");
          +    final String updateSql = "update \"foo\".\"bar\"\n"
          +        + "set \"name\" = 'Ruby'\n"
          +        + "where \"empid\" = 100";
          +    with.query(updateSql)
          +        .updates(3);
          +    with.query("select * from \"foo\".\"bar\"")
          +        .returnsUnordered(
          +            "empid=0; deptno=0; name=first; salary=0.0; commission=null",
          +            "empid=100; deptno=10; name=Ruby; salary=10000.0; commission=1000",
          +            "empid=110; deptno=10; name=Theodore; salary=11500.0; commission=250",
          +            "empid=150; deptno=10; name=Sebastian; salary=7000.0; commission=null",
          +            "empid=200; deptno=20; name=Eric; salary=8000.0; commission=500");
          +  }
          +
             /**
              * Creates the post processor routine to be applied against a Connection.
              *
          
          Show
          julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/568/commits/d3d56a8ae67ea5af2b6dd5371f5b364938bab2b9: Looks better. It doesn't seem possible to execute UPDATE in tests (see below), but can you add a test where you call connection.prepareStatement().getParameterMetaData() and make sure that the bind variable has the right type? Here is the update test I tried to add; it gave an error (not your fault, and we should fix it sometime): diff --git a/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java b/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java index da6facc..898e89e 100644 --- a/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java +++ b/core/src/test/java/org/apache/calcite/test/JdbcFrontLinqBackTest.java @@ -294,6 +294,35 @@ public Object apply(CalciteConnection c) { "name=first; C=1"); } + @Test public void testUpdate() { + final List<JdbcTest.Employee> employees = new ArrayList<>(); + CalciteAssert.AssertThat with = mutable(employees); + with.query("select * from \"foo\".\"bar\"") + .returnsUnordered( + "empid=0; deptno=0; name=first; salary=0.0; commission=null"); + with.query("insert into \"foo\".\"bar\" select * from \"hr\".\"emps\"") + .updates(4); + with.query("select * from \"foo\".\"bar\"") + .returnsUnordered( + "empid=0; deptno=0; name=first; salary=0.0; commission=null", + "empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000", + "empid=110; deptno=10; name=Theodore; salary=11500.0; commission=250", + "empid=150; deptno=10; name=Sebastian; salary=7000.0; commission=null", + "empid=200; deptno=20; name=Eric; salary=8000.0; commission=500"); + final String updateSql = "update \"foo\".\"bar\"\n" + + "set \"name\" = 'Ruby'\n" + + "where \"empid\" = 100"; + with.query(updateSql) + .updates(3); + with.query("select * from \"foo\".\"bar\"") + .returnsUnordered( + "empid=0; deptno=0; name=first; salary=0.0; commission=null", + "empid=100; deptno=10; name=Ruby; salary=10000.0; commission=1000", + "empid=110; deptno=10; name=Theodore; salary=11500.0; commission=250", + "empid=150; deptno=10; name=Sebastian; salary=7000.0; commission=null", + "empid=200; deptno=20; name=Eric; salary=8000.0; commission=500"); + } + /** * Creates the post processor routine to be applied against a Connection. *
          Hide
          eolivelli Enrico Olivelli added a comment -

          Julian Hyde I think I have finally found the right fix for this.
          The problem is that expandStar loses information about pre-validated node types.
          I have updated the patch

          https://github.com/apache/calcite/pull/568/files

          thanks

          Show
          eolivelli Enrico Olivelli added a comment - Julian Hyde I think I have finally found the right fix for this. The problem is that expandStar loses information about pre-validated node types. I have updated the patch https://github.com/apache/calcite/pull/568/files thanks
          Hide
          eolivelli Enrico Olivelli added a comment -

          The more I am working on Calcite the more I am getting a general view at least for the converter/valudator/planner part. While working on the patch for dynamic parameters for fetch/offset I think I have understood what you are suggesting.
          I will update this patch and send the other one.

          Show
          eolivelli Enrico Olivelli added a comment - The more I am working on Calcite the more I am getting a general view at least for the converter/valudator/planner part. While working on the patch for dynamic parameters for fetch/offset I think I have understood what you are suggesting. I will update this patch and send the other one.
          Hide
          julianhyde Julian Hyde added a comment -

          Allowing parameters to have unknown type would be a big task. There would be breakage at various places in the stack (e.g. the JDBC driver). Also, if these values were used in functions we would need to find a way to overload functions on unknown types. Can you log a separate JIRA case for it?

          Reviewing https://github.com/apache/calcite/pull/568/commits/66a4f3b0c5e50a171af6c1e1bbab28a5cba04b2b: SqlToRelConverter must treat the validator as read-only, so calling setValidatedNodeType is not OK. Did you try changing the validator's tree-walk, as I suggested?

          Show
          julianhyde Julian Hyde added a comment - Allowing parameters to have unknown type would be a big task. There would be breakage at various places in the stack (e.g. the JDBC driver). Also, if these values were used in functions we would need to find a way to overload functions on unknown types. Can you log a separate JIRA case for it? Reviewing https://github.com/apache/calcite/pull/568/commits/66a4f3b0c5e50a171af6c1e1bbab28a5cba04b2b: SqlToRelConverter must treat the validator as read-only, so calling setValidatedNodeType is not OK. Did you try changing the validator's tree-walk, as I suggested?
          Hide
          eolivelli Enrico Olivelli added a comment -

          Maybe another alternative option would be to implement a flag to allow unknown type for dynamic parameters, letting the implementation to deal with casts

          this will allow queries like
          select ? as 'foo', a,b from my table which are not possible at the moment

          Show
          eolivelli Enrico Olivelli added a comment - Maybe another alternative option would be to implement a flag to allow unknown type for dynamic parameters, letting the implementation to deal with casts this will allow queries like select ? as 'foo', a,b from my table which are not possible at the moment
          Hide
          eolivelli Enrico Olivelli added a comment - - edited

          Julian Hyde I found a simpler approach, that is to use setValidatedNodeType in convertUpdate to instruct the validator about inferred type of additional SqlNodes in the selectlist for the update.

          This way the change is minimal: no changes to signatures of methods.
          but I did not change the code according to your suggestion "You might need to change the tree-walk so that inferUnknownTypes gets called everywhere"

          https://github.com/apache/calcite/pull/568/files

          I appreciate your help

          Show
          eolivelli Enrico Olivelli added a comment - - edited Julian Hyde I found a simpler approach, that is to use setValidatedNodeType in convertUpdate to instruct the validator about inferred type of additional SqlNodes in the selectlist for the update. This way the change is minimal: no changes to signatures of methods. but I did not change the code according to your suggestion "You might need to change the tree-walk so that inferUnknownTypes gets called everywhere" https://github.com/apache/calcite/pull/568/files I appreciate your help
          Hide
          eolivelli Enrico Olivelli added a comment -

          Thanks
          I will debug deeper SqlToRelConverter.
          I am getting the same error for some other kind of INSERT I will add testcases as well.

          Will send a new patch as soon as I find the culprit

          Show
          eolivelli Enrico Olivelli added a comment - Thanks I will debug deeper SqlToRelConverter. I am getting the same error for some other kind of INSERT I will add testcases as well. Will send a new patch as soon as I find the culprit
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing https://github.com/apache/calcite/pull/568/commits/f7ada9cd493d05100cce9e2791012919656ca4eb. I'm surprised you needed to add a parameter to expandStar. The validator should be able to deduce the type of every column. If it can't, the expression is not valid SQL. (That sounds strict but it's easier this way.) In SqlToRelConverter you should be able to ask the validator what the validated type of a particular expression was. You might need to change the tree-walk so that inferUnknownTypes gets called everywhere (that's probably why this fails for UPDATE but works for SELECT).

          Show
          julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/568/commits/f7ada9cd493d05100cce9e2791012919656ca4eb . I'm surprised you needed to add a parameter to expandStar. The validator should be able to deduce the type of every column. If it can't, the expression is not valid SQL. (That sounds strict but it's easier this way.) In SqlToRelConverter you should be able to ask the validator what the validated type of a particular expression was. You might need to change the tree-walk so that inferUnknownTypes gets called everywhere (that's probably why this fails for UPDATE but works for SELECT).
          Hide
          eolivelli Enrico Olivelli added a comment -

          I have tried to solve this problem, this is the patch
          https://github.com/apache/calcite/pull/568

          Show
          eolivelli Enrico Olivelli added a comment - I have tried to solve this problem, this is the patch https://github.com/apache/calcite/pull/568
          Hide
          eolivelli Enrico Olivelli added a comment -

          Can you give me some hint?
          I will be happy to submit a patch.
          I am still a newbie in Calcite.
          How is it supposed to be bound the type for a dynamic parameter?
          In this case I think we could derive the type from the column which is going to be updated/filtered.
          Another approach would be to drop the assertion in this special case. I don't think it is bad to have a unknown type in this case, but maybe I am missing something.

          /cc Michael Mior

          Show
          eolivelli Enrico Olivelli added a comment - Can you give me some hint? I will be happy to submit a patch. I am still a newbie in Calcite. How is it supposed to be bound the type for a dynamic parameter? In this case I think we could derive the type from the column which is going to be updated/filtered. Another approach would be to drop the assertion in this special case. I don't think it is bad to have a unknown type in this case, but maybe I am missing something. /cc Michael Mior
          Hide
          julianhyde Julian Hyde added a comment -

          Sorry, I don't. Contributions welcome.

          Show
          julianhyde Julian Hyde added a comment - Sorry, I don't. Contributions welcome.
          Hide
          eolivelli Enrico Olivelli added a comment -

          The error is even with ProjectableFilterableTable

          Show
          eolivelli Enrico Olivelli added a comment - The error is even with ProjectableFilterableTable
          Hide
          eolivelli Enrico Olivelli added a comment -

          Julian Hyde Do you have time to take at look at this too? Thanks

          Show
          eolivelli Enrico Olivelli added a comment - Julian Hyde Do you have time to take at look at this too? Thanks

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              eolivelli Enrico Olivelli
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development