[jira] [Commented] (DERBY-6920) Add input support for new date and time classes

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (DERBY-6920) Add input support for new date and time classes

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/DERBY-6920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15852828#comment-15852828 ]

Rick Hillegas commented on DERBY-6920:
--------------------------------------

Hi Philippe,

Thanks again for the patch. Here are my review comments.

1) The code looks good to me. I expect that tests will show that it correctly handles the client driver PreparedStatement/CallableStatement.setObject(int parameterIndex, Object x) method for LocalDate, LocalTime, and LocalDateTime. It would be a reasonable increment to checkin if that setObject() overload also handled  OffsetTime and OffsetDateTime.

2) I recommend also adding support for the corresponding method in the embedded driver. You can separate the work on the two drivers if you want to. However, I suspect that you will end up fighting the test harness if you approach the problem that way. That is because the default behavior of the test harness is to run a test case in both client and embedded modes.

3) I don't think that ResultSetTest is the right place to add test cases for PreparedStatement/CallableStatement methods. In any event, the change to ResultSetTest broke 2 tests when I applied the patch and ran the full regression test suite. These were the errors:'

{noformat}
There were 2 errors:
1) testJDBC4_1(org.apache.derbyTesting.functionTests.tests.jdbc4.ResultSetTest)java.sql.SQLDataException: An attempt was made to get a data value of type 'DATE' from a data value of type 'java.time.LocalDate'.
        at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:84)
        at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:141)
        at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:225)
        at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:220)
        at org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(EmbedConnection.java:3210)
        at org.apache.derby.impl.jdbc.ConnectionChild.newSQLException(ConnectionChild.java:155)
        at org.apache.derby.impl.jdbc.EmbedPreparedStatement.dataTypeConversion(EmbedPreparedStatement.java:1714)
        at org.apache.derby.impl.jdbc.EmbedPreparedStatement.setObject(EmbedPreparedStatement.java:1384)
        at org.apache.derbyTesting.functionTests.tests.jdbc4.ResultSetTest.testJDBC4_1(ResultSetTest.java:2070)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:120)
        at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:443)
        at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:460)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:58)
Caused by: ERROR 22005: An attempt was made to get a data value of type 'DATE' from a data value of type 'java.time.LocalDate'.
        at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:290)
        at org.apache.derby.impl.jdbc.SQLExceptionFactory.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory.java:170)
        at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:75)
        ... 41 more
2) testJDBC4_1(org.apache.derbyTesting.functionTests.tests.jdbc4.ResultSetTest)java.sql.SQLTransactionRollbackException: FUNCTION 'MAKEBLOB' already exists.
        at org.apache.derby.client.am.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:115)
        at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:321)
        at org.apache.derby.client.am.ClientPreparedStatement.execute(ClientPreparedStatement.java:1737)
        at org.apache.derbyTesting.functionTests.tests.jdbc4.ResultSetTest.testJDBC4_1(ResultSetTest.java:1984)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:120)
        at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:443)
        at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:460)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:58)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:58)
Caused by: ERROR X0Y68: FUNCTION 'MAKEBLOB' already exists.
        at org.apache.derby.client.am.ClientStatement.completeExecute(ClientStatement.java:1861)
        at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:323)
        at org.apache.derby.client.net.NetStatementReply.readExecute(NetStatementReply.java:72)
        at org.apache.derby.client.net.StatementReply.readExecute(StatementReply.java:59)
        at org.apache.derby.client.net.NetPreparedStatement.readExecute_(NetPreparedStatement.java:167)
        at org.apache.derby.client.am.ClientPreparedStatement.readExecute(ClientPreparedStatement.java:1965)
        at org.apache.derby.client.am.ClientPreparedStatement.flowExecute(ClientPreparedStatement.java:2254)
        at org.apache.derby.client.am.ClientPreparedStatement.executeX(ClientPreparedStatement.java:1743)
        at org.apache.derby.client.am.ClientPreparedStatement.execute(ClientPreparedStatement.java:1728)
        ... 43 more

FAILURES!!!
Tests run: 13901,  Failures: 0,  Errors: 2
{noformat}

4) I recommend that you discard the changes to ResultSetTest and, instead, add a new test class for the JDBC 4.2 date/time support. The new test class should be added to the org.apache.derbyTesting.functionTests.tests.jdbc4._Suite. I recommend PreparedStatementTest42 as a model for how to write the test.

5) If you do that additional work, then I think that this patch will be a logical increment which we can checkin. You will then have a base to build on and a pattern to follow as you add support for the other setObject() overloads and the ResultSet methods described by the spec attached to DERBY-6445.

One other point: This project will involve many patches. I recommend that you follow a regular naming convention for your patches so that you and your reviewers can tell at a glance which patch does what. Take a look at DERBY-6856 for an example of such a convention.

Please do not hesitate to ask questions as you proceed. Derby is a complicated codebase and the test harness can be tricky to work with.

Thanks again!
-Rick


> Add input support for new date and time classes
> -----------------------------------------------
>
>                 Key: DERBY-6920
>                 URL: https://issues.apache.org/jira/browse/DERBY-6920
>             Project: Derby
>          Issue Type: Improvement
>          Components: JDBC
>            Reporter: Philippe Marschall
>         Attachments: jsr-310-input.diff
>
>
> This is the first half of DERBY-6445. The patch adds support for the new date and time classes for input only. I may add output support later but for now splitting it up makes for a smaller, easier to review and implement patch.
> A couple of notes about the implementation:
> * as the project now requires Java 1.8 I added the code directly to the classes
> * the existing tests are expanded
> * I avoided calling the provided #valueOf conversion methods for several reasons:
> ** LocalTime has nanosecond resolution but java.sql.Time only has millisecond resolution
> ** LocalDateTime can represent timestamps that can not be represented by java.sql.Timestamp because they fall into a daylight saving time transition
> ** Performance should be much better since creating a DateTimeValue instance is a simple matter of calling a few getters. No calculation or object creation is involved.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)