Derby
  1. Derby
  2. DERBY-5143

Remove unnecessary copying of the map in getTypeMap()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.8.1.2
    • Fix Version/s: 10.8.1.2
    • Component/s: JDBC
    • Labels:
      None

      Description

      The JDBC 4.0 Connection classes implement getTypeMap() by calling super.getTypeMap() and copying the resulting map. This is done to prevent an unchecked compiler warning that we would see if we simply returned super.getTypeMap() in this method. It would be cheaper and simpler to return super.getTypeMap() and ignore the warning.

      1. getTypeMap-warning.diff
        4 kB
        Knut Anders Hatlen
      2. derby-5143-1a.diff
        6 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Thank you, Lily, for testing the patch. I also ran suites.All and derbyall with Java 6 without any failures.

        Committed revision 1083917.

        Show
        Knut Anders Hatlen added a comment - Thank you, Lily, for testing the patch. I also ran suites.All and derbyall with Java 6 without any failures. Committed revision 1083917.
        Hide
        Lily Wei added a comment -

        Thank you so much, Knut. I run derby-5143-1a.diff patch for ConnectionTest test against JDBC 3.0 and JDBC 4.1. +1 for the patch.

        Show
        Lily Wei added a comment - Thank you so much, Knut. I run derby-5143-1a.diff patch for ConnectionTest test against JDBC 3.0 and JDBC 4.1. +1 for the patch.
        Hide
        Knut Anders Hatlen added a comment -

        Attaching a new patch (derby-5143-1a.diff) with the following changes from the initial patch:

        • simplified the getTypeMap() overrides a little more
        • made the client implementation return EMPTY_MAP (immutable) instead of an empty HashMap (mutable), to match the embedded implementation
        • updated ConnectionTest to expect the returned map to be immutable
        Show
        Knut Anders Hatlen added a comment - Attaching a new patch (derby-5143-1a.diff) with the following changes from the initial patch: simplified the getTypeMap() overrides a little more made the client implementation return EMPTY_MAP (immutable) instead of an empty HashMap (mutable), to match the embedded implementation updated ConnectionTest to expect the returned map to be immutable
        Hide
        Knut Anders Hatlen added a comment -

        I think the reason why we added the getTypeMap() method in the first place was that the compilation of NetConnection40 would generate the following warning:

        Compiling 1 source file to /code/derby/trunk0/classes
        /code/derby/trunk0/java/client/org/apache/derby/client/net/NetConnection40.java:48: warning: getTypeMap() in org.apache.derby.client.am.Connection implements getTypeMap() in java.sql.Connection; return type requires unchecked conversion
        found : java.util.Map
        required: java.util.Map<java.lang.String,java.lang.Class<?>>
        public class NetConnection40 extends org.apache.derby.client.net.NetConnection {
        1 warning

        And suppressing unchecked warnings for the entire class wasn't considered a good alternative. There may be other ways to silence this specific warning only, but I'm not aware of any.

        You're of course right that Connection.getTypeMap() should return Collections.EMPTY_MAP. I thought it already did, but that's only on the embedded driver. Will fix that too.

        Show
        Knut Anders Hatlen added a comment - I think the reason why we added the getTypeMap() method in the first place was that the compilation of NetConnection40 would generate the following warning: Compiling 1 source file to /code/derby/trunk0/classes /code/derby/trunk0/java/client/org/apache/derby/client/net/NetConnection40.java:48: warning: getTypeMap() in org.apache.derby.client.am.Connection implements getTypeMap() in java.sql.Connection; return type requires unchecked conversion found : java.util.Map required: java.util.Map<java.lang.String,java.lang.Class<?>> public class NetConnection40 extends org.apache.derby.client.net.NetConnection { 1 warning And suppressing unchecked warnings for the entire class wasn't considered a good alternative. There may be other ways to silence this specific warning only, but I'm not aware of any. You're of course right that Connection.getTypeMap() should return Collections.EMPTY_MAP. I thought it already did, but that's only on the embedded driver. Will fix that too.
        Hide
        Dave Brosius added a comment - - edited

        It seems to me like the entire NetConnection40.getTypeMap method can be removed, as the parent class always just returns an empty Map. In fact, Connection.getTypeMap, could just return a static field Map defined as

        static final Map typeMap = Collections.unmodfiableSet(new HashSet());

        or perhaps even better, just return Collections.EMPTY_SET;

        Show
        Dave Brosius added a comment - - edited It seems to me like the entire NetConnection40.getTypeMap method can be removed, as the parent class always just returns an empty Map. In fact, Connection.getTypeMap, could just return a static field Map defined as static final Map typeMap = Collections.unmodfiableSet(new HashSet()); or perhaps even better, just return Collections.EMPTY_SET;
        Hide
        Knut Anders Hatlen added a comment -

        The attached patch shows how to silence the compiler warning without copying the map.

        A side effect of this change is that the returned map in the JDBC 4.0 driver is exactly the same as the one that would have been returned by the JDBC 3.0 driver, and it is immutable. One of the JDBC 4.0 tests (ConnectionTest.testGetTypeMapReturnsAsExpected) calls put() on the returned map, and therefore starts failing with an UnsupportedOperationException now that the returned map isn't mutable. I think the test should be changed, and the map should stay immutable.

        Show
        Knut Anders Hatlen added a comment - The attached patch shows how to silence the compiler warning without copying the map. A side effect of this change is that the returned map in the JDBC 4.0 driver is exactly the same as the one that would have been returned by the JDBC 3.0 driver, and it is immutable. One of the JDBC 4.0 tests (ConnectionTest.testGetTypeMapReturnsAsExpected) calls put() on the returned map, and therefore starts failing with an UnsupportedOperationException now that the returned map isn't mutable. I think the test should be changed, and the map should stay immutable.
        Hide
        Knut Anders Hatlen added a comment -

        One unintended side effect from the copying is that the returned map is mutable when using the JDBC 4.0 driver, whereas it's immutable with the JDBC 3.0 driver.

        Show
        Knut Anders Hatlen added a comment - One unintended side effect from the copying is that the returned map is mutable when using the JDBC 4.0 driver, whereas it's immutable with the JDBC 3.0 driver.

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development