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

Missing implementation of ResultSetMetaData.getColumnClassName(int)

    Details

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

      Description

      We are running into an obstacle using a very simple Calcite in memory table.
      It manifests itself in getting the column classname from ResultSetMetaData. It seems easy enough to fix, at least for our purposes. Here is a proposed patch relative to Git master within the last hour:

      diff --git a/avatica/src/main/java/org/apache/calcite/avatica/ColumnMetaData.java b/avatica/src/main/java/org/apache/calcite/avatica/ColumnMetaData.java
      index 102590e..fca92b4 100644
      --- a/avatica/src/main/java/org/apache/calcite/avatica/ColumnMetaData.java
      +++ b/avatica/src/main/java/org/apache/calcite/avatica/ColumnMetaData.java
      @@ -106,7 +106,8 @@ public ColumnMetaData(
           this.readOnly = readOnly;
           this.writable = writable;
           this.definitelyWritable = definitelyWritable;
      -    this.columnClassName = columnClassName;
      +    this.columnClassName = columnClassName != null
      +        ? columnClassName : type.representation.clazz.getName();
         }
       
         private static <T> T first(T t0, T t1) {
      diff --git a/core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java b/core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
      index 58ebe99..3898422 100644
      --- a/core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
      +++ b/core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
      @@ -77,6 +77,8 @@
           assertThat(resultSetMetaData.getColumnName(1), equalTo("A"));
           assertThat(resultSetMetaData.getTableName(1), equalTo("SAMPLE"));
           assertThat(resultSetMetaData.getSchemaName(1), nullValue());
      +    assertThat(resultSetMetaData.getColumnClassName(1),
      +               equalTo("java.lang.String"));
           // Per JDBC, column name should be null. But DBUnit requires every column
           // to have a name, so the driver uses the label.
           assertThat(resultSetMetaData.getColumnName(2), equalTo("EXPR$1"));
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Closing now that 1.0.0-incubating has been released.

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

        Oops. Copy-paste error.

        Show
        julianhyde Julian Hyde added a comment - Oops. Copy-paste error.
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        INTERVAL_YEAR_MONTH(Types.OTHER, Boolean.class),
        + INTERVAL_DAY_TIME(Types.OTHER, Boolean.class),
        +

        Are you sure?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - INTERVAL_YEAR_MONTH(Types.OTHER, Boolean.class), + INTERVAL_DAY_TIME(Types.OTHER, Boolean.class), + Are you sure?
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/019a16f9 .
        Hide
        julianhyde Julian Hyde added a comment -

        I don't recall why we have type and fieldType, but I'm pretty sure there was a good reason - and I'm sure a test case would break if we tried to combine them.

        getClassName should be fairly simple - it basically indicates the "user type" of the returned value, and hence the preferred JDBC method (e.g. getInt or getString or getTimestamp etc.). Nothing to do with the internal type (e.g. internally we handle SQL TIMESTAMP values as Java long values, but externally they are java.sql.Timestamp.) That user type should follow in a very predictable way from the SQL type.

        It should be based on AvaticaType.type, i.e. ColumnMetaData.type.type. Because we can derive from the type id, we don't need a AvaticaType.className field (until we support custom type mappings).

        Show
        julianhyde Julian Hyde added a comment - I don't recall why we have type and fieldType, but I'm pretty sure there was a good reason - and I'm sure a test case would break if we tried to combine them. getClassName should be fairly simple - it basically indicates the "user type" of the returned value, and hence the preferred JDBC method (e.g. getInt or getString or getTimestamp etc.). Nothing to do with the internal type (e.g. internally we handle SQL TIMESTAMP values as Java long values, but externally they are java.sql.Timestamp.) That user type should follow in a very predictable way from the SQL type. It should be based on AvaticaType.type, i.e. ColumnMetaData.type.type. Because we can derive from the type id, we don't need a AvaticaType.className field (until we support custom type mappings).
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        Frankly speaking, I have no idea how this part of the code works.
        It is strange for me we have two RelDataType}}s there: {{RelDataType type, RelDataType fieldType.

        Julian Hyde, any clue how CalcitePrepareImpl#getClassName should look like?

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - Frankly speaking, I have no idea how this part of the code works. It is strange for me we have two RelDataType}}s there: {{RelDataType type, RelDataType fieldType . Julian Hyde , any clue how CalcitePrepareImpl#getClassName should look like?
        Hide
        knut-apache@forkalsrud.org Knut Forkalsrud added a comment -

        I'll defer to your best judgement. I think org.apache.calcite.prepare.CalcitePrepareImpl#getClassName would need access to a JavaTypeFactory to be able to to the job, in which the implementation would look something like:

        private static String getClassName(JavaTypeFactory typeFactory, RelDataType type) {
          return ColumnMetaData.Rep.of(typeFactory.getJavaClass(type)).clazz.getName();
        }
        
        Show
        knut-apache@forkalsrud.org Knut Forkalsrud added a comment - I'll defer to your best judgement. I think org.apache.calcite.prepare.CalcitePrepareImpl#getClassName would need access to a JavaTypeFactory to be able to to the job, in which the implementation would look something like: private static String getClassName(JavaTypeFactory typeFactory, RelDataType type) { return ColumnMetaData.Rep.of(typeFactory.getJavaClass(type)).clazz.getName(); }
        Hide
        vladimirsitnikov Vladimir Sitnikov added a comment -

        The patch looks odd.

        I think org.apache.calcite.prepare.CalcitePrepareImpl#getClassName should be patched instead.

        Show
        vladimirsitnikov Vladimir Sitnikov added a comment - The patch looks odd. I think org.apache.calcite.prepare.CalcitePrepareImpl#getClassName should be patched instead.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            knut-apache@forkalsrud.org Knut Forkalsrud
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development