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

Serializing Signature.internalParameters can cause exceptions

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0-incubating
    • Component/s: None
    • Labels:
      None

      Description

      Observed exceptions:

      java.lang.AssertionError: null
      at org.apache.calcite.rel.type.RelDataTypeImpl.getFieldList(RelDataTypeImpl.java:138) ~[calcite-core-1.0.0-incubating-SNAPSHOT.jar:1.0.0-incubating-SNAPSHOT]
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_25]
      at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_25]
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.8.0_25]
      at java.lang.reflect.Method.invoke(Method.java:483) ~[na:1.8.0_25]
      at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:466) ~[jackson-databind-2.4.2.jar:2.4.2]
      

      And in https://github.com/devth/calcite-map-demo:

      java.lang.RuntimeException: com.fasterxml.jackson.databind.JsonMappingException: No serializer found for class org.apache.calcite.sql.type.SqlTypeExplicitPrecedenceList and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationConfig.SerializationFeature.FAIL_ON_EMPTY_BEANS) ) (through reference chain: org.apache.calcite.avatica.remote.ResultSetResponse["signature"]->org.apache.calcite.avatica.Signature["internalParameters"]->java.util.LinkedHashMap["v0stashed"]->com.google.common.collect.RegularImmutableList[0]->org.apache.calcite.rex.RexCall["operands"]->com.google.common.collect.RegularImmutableList[0]->org.apache.calcite.rex.RexInputRef["type"]->org.apache.calcite.sql.type.MapSqlType["keyType"]->org.apache.calcite.sql.type.BasicSqlType["precedenceList"])
              at org.apache.calcite.avatica.remote.JsonHandler.handle(JsonHandler.java:61) ~[calcite-avatica-1.0.0-incubating.jar:1.0.0-incubating]
              at org.apache.calcite.avatica.remote.JsonHandler.apply(JsonHandler.java:46) ~[calcite-avatica-1.0.0-incubating.jar:1.0.0-incubating]
              at org.apache.calcite.avatica.server.AvaticaHandler.handle(AvaticaHandler.java:47) ~[calcite-avatica-server-1.0.0-incubating.jar:1.0.0-incubating]
      

      Pull request:
      https://github.com/apache/incubator-calcite/pull/48

        Activity

        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Trevor Hartman, can you please split pull request into two separate ones?

        Julian Hyde, do we need to remove annotation @JsonProperty("internalParameters") Map<String, Object> internalParameters as well?

            @JsonCreator
            public Signature(@JsonProperty("columns") List<ColumnMetaData> columns,
                @JsonProperty("sql") String sql,
                @JsonProperty("parameters") List<AvaticaParameter> parameters,
                @JsonProperty("internalParameters") Map<String, Object>
                    internalParameters,
                @JsonProperty("cursorFactory") CursorFactory cursorFactory) {
              this.columns = columns;
              this.sql = sql;
              this.parameters = parameters;
              this.internalParameters = internalParameters;
        
        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Trevor Hartman , can you please split pull request into two separate ones? Julian Hyde , do we need to remove annotation @JsonProperty("internalParameters") Map<String, Object> internalParameters as well? @JsonCreator public Signature(@JsonProperty( "columns" ) List<ColumnMetaData> columns, @JsonProperty( "sql" ) String sql, @JsonProperty( "parameters" ) List<AvaticaParameter> parameters, @JsonProperty( "internalParameters" ) Map< String , Object > internalParameters, @JsonProperty( "cursorFactory" ) CursorFactory cursorFactory) { this .columns = columns; this .sql = sql; this .parameters = parameters; this .internalParameters = internalParameters;
        Hide
        devth Trevor Hartman added a comment - - edited

        Vladimir Sitnikov, I accidentally clobbered master branch when doing the other PR. It's fixed now. FWIW I experimented with removing @JsonProperty("internalParameters") but it made no difference in JSON output.

        Show
        devth Trevor Hartman added a comment - - edited Vladimir Sitnikov , I accidentally clobbered master branch when doing the other PR. It's fixed now. FWIW I experimented with removing @JsonProperty("internalParameters") but it made no difference in JSON output.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Well, I have no idea how it should work and I have no idea if it will fail with some kind of NPE if stuffed with "non existing @JsonProperty("internalParameters")" either.

        Maybe we should just add a safeguard emptymap: this.internalParameters = internalParameters == null ? Collections.emptyMap internalParameters.
        What do you think?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Well, I have no idea how it should work and I have no idea if it will fail with some kind of NPE if stuffed with "non existing @JsonProperty("internalParameters")" either. Maybe we should just add a safeguard emptymap: this.internalParameters = internalParameters == null ? Collections.emptyMap internalParameters . What do you think?
        Hide
        julianhyde Julian Hyde added a comment - - edited

        I suggest the following:

            /** Creates a Signature. */
            public Signature(List<ColumnMetaData> columns,
                String sql,
                List<AvaticaParameter> parameters,
                Map<String, Object> internalParameters,
                CursorFactory cursorFactory) {
              this.columns = columns;
              this.sql = sql;
              this.parameters = parameters;
              this.internalParameters = internalParameters;
              this.cursorFactory = cursorFactory;
              assert internalParameters != null;
            }
        
            /** Used by Jackson to create a Signature by de-serializing JSON. */
            @JsonCreator
            public static Signature create(
                @JsonProperty("columns") List<ColumnMetaData> columns,
                @JsonProperty("sql") String sql,
                @JsonProperty("parameters") List<AvaticaParameter> parameters,
                @JsonProperty("cursorFactory") CursorFactory cursorFactory) {
              return new Signature(columns, sql, parameters,
                  Collections.<String, Object>emptyMap(), cursorFactory);
            }
        

        It makes it clear which interface is being used to de-serialize JSON.

        Show
        julianhyde Julian Hyde added a comment - - edited I suggest the following: /** Creates a Signature. */ public Signature(List<ColumnMetaData> columns, String sql, List<AvaticaParameter> parameters, Map< String , Object > internalParameters, CursorFactory cursorFactory) { this .columns = columns; this .sql = sql; this .parameters = parameters; this .internalParameters = internalParameters; this .cursorFactory = cursorFactory; assert internalParameters != null ; } /** Used by Jackson to create a Signature by de-serializing JSON. */ @JsonCreator public static Signature create( @JsonProperty( "columns" ) List<ColumnMetaData> columns, @JsonProperty( "sql" ) String sql, @JsonProperty( "parameters" ) List<AvaticaParameter> parameters, @JsonProperty( "cursorFactory" ) CursorFactory cursorFactory) { return new Signature(columns, sql, parameters, Collections.< String , Object >emptyMap(), cursorFactory); } It makes it clear which interface is being used to de-serialize JSON.
        Hide
        devth Trevor Hartman added a comment -

        My tests passed against Julian Hyde's proposed solution. I updated the PR.

        Show
        devth Trevor Hartman added a comment - My tests passed against Julian Hyde 's proposed solution. I updated the PR.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/e237804b0e4d7ac17249708a5cf823e0c1fe1625 .
        Hide
        ndimiduk Nick Dimiduk added a comment -

        Hi folks, it appears this commit broke the build.

        [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project calcite-core: Compilation failure: Compilation failure:
        [ERROR] /Users/ndimiduk/repos/incubator-calcite/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java:[24,42] cannot find symbol
        [ERROR] symbol:   class SqlParserImpl
        [ERROR] location: package org.apache.calcite.sql.parser.impl
        [ERROR] /Users/ndimiduk/repos/incubator-calcite/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java:[201,50] cannot find symbol
        [ERROR] symbol:   variable SqlParserImpl
        [ERROR] location: class org.apache.calcite.sql.parser.SqlParser.ConfigBuilder
        

        According to git bisect:

        $ git bisect start origin/master calcite-1.0.0-incubating
        $ git bisect run mvn clean package -DskipTests
        ...
        4b76c50d88c7524ef4c4a29a77cebcde8d317ca7 is the first bad commit
        commit 4b76c50d88c7524ef4c4a29a77cebcde8d317ca7
        Author: Julian Hyde <jhyde@apache.org>
        Date:   Sun Feb 8 14:30:41 2015 -0800
        
            [CALCITE-586] Prevent JSON serialization of Signature.internalParameters (Trevor Hartman)
            
            (That should have been the commit message for the previous commit.)
            
            Update history
            
            Close apache/incubator-calcite#48
        
        :040000 040000 9911637d375dea6e05ce1ef1cc548ec42b5971df e2149323a2d7d47b3d86a35371f169a8d59bfadb M      doc
        bisect run success
        
        Show
        ndimiduk Nick Dimiduk added a comment - Hi folks, it appears this commit broke the build. [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project calcite-core: Compilation failure: Compilation failure: [ERROR] /Users/ndimiduk/repos/incubator-calcite/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java:[24,42] cannot find symbol [ERROR] symbol: class SqlParserImpl [ERROR] location: package org.apache.calcite.sql.parser.impl [ERROR] /Users/ndimiduk/repos/incubator-calcite/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java:[201,50] cannot find symbol [ERROR] symbol: variable SqlParserImpl [ERROR] location: class org.apache.calcite.sql.parser.SqlParser.ConfigBuilder According to git bisect: $ git bisect start origin/master calcite-1.0.0-incubating $ git bisect run mvn clean package -DskipTests ... 4b76c50d88c7524ef4c4a29a77cebcde8d317ca7 is the first bad commit commit 4b76c50d88c7524ef4c4a29a77cebcde8d317ca7 Author: Julian Hyde <jhyde@apache.org> Date: Sun Feb 8 14:30:41 2015 -0800 [CALCITE-586] Prevent JSON serialization of Signature.internalParameters (Trevor Hartman) (That should have been the commit message for the previous commit.) Update history Close apache/incubator-calcite#48 :040000 040000 9911637d375dea6e05ce1ef1cc548ec42b5971df e2149323a2d7d47b3d86a35371f169a8d59bfadb M doc bisect run success
        Show
        devth Trevor Hartman added a comment - Nick Dimiduk see https://issues.apache.org/jira/browse/CALCITE-553
        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.1.0-incubating has been released.

        Show
        julianhyde Julian Hyde added a comment - Closing now that 1.1.0-incubating has been released.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            devth Trevor Hartman
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development