Add support for LocalDate, LocalTime and LocalDateTime to be passed as 'type' in ResultSet.getObject()

[gordthompson]

Issue #744 - support for rs.getObject(n, java.time.LocalDateTime.class)

[codecov-io]

Codecov Report

Merging #749 into dev will increase coverage by <.01%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##                dev     #749      +/-   ##
============================================
+ Coverage     48.32%   48.32%   +<.01%     
- Complexity     2774     2780       +6     
============================================
  Files           116      116              
  Lines         27871    27883      +12     
  Branches       4631     4635       +4     
============================================
+ Hits          13468    13474       +6     
- Misses        12215    12233      +18     
+ Partials       2188     2176      -12

Flag	Coverage Δ	Complexity Δ
#JDBC42	47.83% <83.33%> (-0.03%)	2736 <0> (+5)
#JDBC43	48.24% <83.33%> (+0.02%)	2775 <0> (+6)	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	32.38% <83.33%> (+0.15%)	252 <0> (+4)	⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	45.05% <0%> (-1.1%)	16% <0%> (ø)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	59.82% <0%> (-0.44%)	87% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	53.37% <0%> (-0.4%)	0% <0%> (ø)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	62.37% <0%> (+0.11%)	0% <0%> (ø)	⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	38.79% <0%> (+0.43%)	43% <0%> (+1%)	⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	45.73% <0%> (+1.96%)	107% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e697a5...1f30188. Read the comment docs.

[cheenamalhotra]

Hi @gordthompson

Thanks for coming up with PR!

We should also add related API for SQLServerResultSet.getLocalDateTime(...) [also to be defined in ISQLServerResultSet interface and overridden].

I would also like to know your thoughts about providing LocalDateTime support as PreparedStatement.setLocalDateTime() as well? Although internally the driver would process it same way, but would it make sense for the sake of LocalDateTime datatype support?

[mrotteveel]

@cheenamalhotra No such method exists in the JDBC (java.sql) API itself, so adding such methods is not very useful, as most people will only be using methods exposed by the JDBC API itself (casting or unwrapping has a lot of caveats when using connection pools for example). I'd urge you to not add them, because it will be simpler and less confusing.

When support for JSR-310 (java.time) objects like LocalDateTime (and others) was added in JDBC 4.2, the JDBC expert group decided that adding specific getXXX/setXXX/updateXXX methods for these types was not necessary.

See this recent comment on the jdbc-spec-discuss mailing list: http://mail.openjdk.java.net/pipermail/jdbc-spec-discuss/2018-July/000361.html by Lance Andersen, the JDBC spec-lead.

[mrotteveel]

I'm not sure if this is correct given JDBC specifies that the time should be rendered in the JVM default time zone. In other words, setting 'now' as a java.sql.Timestamp and retrieving it as a LocalDateTime in a datetime (no time zone info) should return the same value if you'd compare their toString() output. This will depend on how the time is stored in SQL Server.

[gordthompson]

@mrotteveel The Timestamp needs to be retrieved as UTC to prevent certain datetime2 values from being corrupted if they are in the "lost hour" when switching from Standard Time to Daylight Time (a.k.a. Summer Time). For example, in my time zone (America/Edmonton), the datetime2 value 2018-03-11T02:00:00 will magically change to 2018-03-11T03:00:00 if we try to retrieve it in the default time zone since there was no 02:00 that morning; the time changed from 01:59 to 03:00.

[mrotteveel]

Clear. You may want to consider skipping timestamp retrieval and instead convert directly from the datetime value to LocalDateTime; although that might also be considered as a future improvement.

[mrotteveel]

Parsing to string and back is inefficient. Consider using Timestamp.toLocalDateTime() (assuming my previous comment is addressed), or LocalDateTime.ofInstant(ts.getInstant(), <zone offset you want>).

[gordthompson]

@mrotteveel Thanks. I went with the latter.

[mrotteveel]

@gordthompson I left two comments regarding the conversion. I also wonder if you considered adding support for LocalTime, OffsetDateTime and OffsetTime which were also added in JDBC 4.2.

[gordthompson]

@mrotteveel I focused on LocalDateTime because that was where people could get stung by the "lost hour" issue (e.g., this SO answer). SQL Server datetime2 really is a Java LocalDateTime (since datetime2 has no time zone component) so I thought it was most important to support that conversion directly.

[gordthompson]

@mrotteveel For (more) completeness I also added support for LocalDate and LocalTime, also achieved by first retrieving a Timestamp as UTC and then converting. I agree that it might be more efficient to bypass the Timestamp and retrieve the values directly from the TDS payload (as I did for a pyodbc Output Converter Function here) , but I strongly suspect that the Java code would then have to do significantly different things depending on the column type from which the value was retrieved. That's a can of worms that I don't have time to open right now, but perhaps it should be noted as a possible future enhancement.