add "connectionDateformat" connection parameter

[gordthompson]

(Prompted by this Stack Overflow question.)

It is not uncommon for applications to try and be database-agnostic by dealing with date/time values as strings. Sqoop seems to be one of those applications.

Normally, using a string date literal like '2017-11-05' is considered safe because the leading year value suggests that the format is yyyy-mm-dd. yyyy-dd-mm is almost unheard of ...

... except with SQL Server. If the SQL Server instance is installed with "British English" (or perhaps "English (UK)"?) as the default locale then the default DATEFORMAT is "dmy". When it is passed a yyyy-xx-xx string literal for a DATETIME value it is interpreted as yyyy-dd-mm, not yyyy-mm-dd:

connUrl = "jdbc:sqlserver://localhost:49242;databaseName=myDb";
try (Connection conn = DriverManager.getConnection(connUrl, uid, pwd)) {
    String sql = "SELECT MONTH(CONVERT(DATETIME, '2017-11-05')) AS month_num";
    System.out.println(sql);
    try (Statement st = conn.createStatement()) {
        ResultSet rs = st.executeQuery(sql);
        rs.next();
        System.out.printf("month_num is %d%n", rs.getInt("month_num"));
    }

result:

SELECT MONTH(CONVERT(DATETIME, '2017-11-05')) AS month_num
month_num is 5

A workaround is to send a SET DATEFORMAT 'ymd' statement immediately after opening the connection. Unfortunately, Sqoop has that option for Oracle connections, but apparently not for other JDBC drivers.

This patch adds a "connectionDateformat" parameter for the connection URL and for a SQLServerDataSource. After adding ;connectionDateformat=ymd ...

connUrl = "jdbc:sqlserver://localhost:49242;databaseName=myDb;connectionDateformat=ymd";
try (Connection conn = DriverManager.getConnection(connUrl, uid, pwd)) {
    String sql = "SELECT MONTH(CONVERT(DATETIME, '2017-11-05')) AS month_num";
    System.out.println(sql);
    try (Statement st = conn.createStatement()) {
        ResultSet rs = st.executeQuery(sql);
        rs.next();
        System.out.printf("month_num is %d%n", rs.getInt("month_num"));
    }

... the result is:

SELECT MONTH(CONVERT(DATETIME, '2017-11-05')) AS month_num
month_num is 11

[codecov-io]

Codecov Report

Merging #542 into dev will decrease coverage by 0.11%.
The diff coverage is 70%.

@@             Coverage Diff              @@
##                dev     #542      +/-   ##
============================================
- Coverage     46.68%   46.56%   -0.12%     
+ Complexity     2228     2221       -7     
============================================
  Files           108      108              
  Lines         25308    25326      +18     
  Branches       4175     4177       +2     
============================================
- Hits          11815    11794      -21     
- Misses        11463    11509      +46     
+ Partials       2030     2023       -7

Flag	Coverage Δ	Complexity Δ
#JDBC41	46.42% <70%> (-0.03%)	2212 <2> (-1)
#JDBC42	46.41% <70%> (-0.17%)	2213 <2> (-11)

Impacted Files	Coverage Δ	Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerResource.java	100% <ø> (ø)	4 <0> (ø)	⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java	77.34% <100%> (+0.12%)	25 <0> (ø)	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	52.7% <100%> (+0.19%)	239 <0> (-1)	⬇️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java	46.56% <100%> (+0.57%)	68 <2> (+2)	⬆️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	45.99% <53.84%> (+0.01%)	278 <0> (+1)	⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	44.94% <0%> (-2.25%)	16% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	51.11% <0%> (-1.49%)	11% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	32.29% <0%> (-0.86%)	237% <0%> (-8%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	53.49% <0%> (-0.37%)	0% <0%> (ø)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	62.93% <0%> (-0.31%)	0% <0%> (ø)
... and 5 more

---

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 5c75147...e19d41b. Read the comment docs.

[ulvii]

I would let the applications handle this case. @v-xiangs , @cheenamalhotra what do you think?

[cheenamalhotra]

Hi @gordthompson Thanks for bringing this change request. I did go through the Stack Overflow discussion, and I understand from your description, changing Default Language to "English" instead of "British English" does make the desired change, and looks a fair solution to the problem.

I would say we want to give complete control over to the developer to decide how and when do they want to make this change. Providing a connection property is an option, but not an ideal solution in this case. If anyone wants to execute this statement on a connection, that can be done programmatically (as it will be in rare cases) and that would not add complexity to the driver.

Also, we would like to seek community's feedback over this connection property and whether it does make a big difference to the driver.

@ajlam @sehrope @TobiasSQL @marschall @David-Engel
Appreciate your thoughts on this new connection property introduction to the driver.

[marschall]

It is not uncommon for applications to try and be database-agnostic by dealing with date/time values as strings.

This is horrible and wrong. The same goes for dealing with date/time values as numbers.

Sqoop seems to be one of those applications.

Then Sqoop needs to be fixed.

[gordthompson]

@cheenamalhotra re: "changing Default Language ... looks a fair solution to the problem"

Except that

1. server configuration changes are often difficult (if not impossible) for humble end users, and
2. such a change could have side-effects.
    

@ulvii re: "let the applications handle this case"
@cheenamalhotra re: "that can be done programmatically"

The whole point of the Pull Request is that doing it "programmatically" does not currently seem to be possible in Sqoop, at least not for SQL Server.
 

@marschall

If string literals for date/time values are so wrong, then (1) why does SQL Server support this in the first place?

CREATE TABLE #tmp (id INT PRIMARY KEY, dt DATETIME);
INSERT INTO #tmp (id, dt) VALUES (1, NULL);
UPDATE #tmp SET dt = '2017-11-05 13:14:15' WHERE id = 1;  -- "horrible and wrong"
SELECT MONTH(dt) AS month_num FROM #tmp WHERE id = 1; 
DROP TABLE #tmp;

And (2) why does it interpret yyyy-xx-xx as yyyy-dd-mm (only for DATETIME, not for DATE or DATETIME2) when the default DATEFORMAT for "British English" is dmy, not ydm.

[sehrope]

I see the utility of this though if you go down the road of adding connection properties for setting the date format, someone could argue for the same thing for numeric formats or timestamp formats. That has nothing to do with them having more consistent formats either. Someone may just want a way to set them to match the format of their input data. Plus as new values get added server side for any of those, the driver would need to be updated to accommodate them (or remove any validation).

Rather than having specific hooks for each property perhaps a generic way to execute a "post-connect" SQL command or set of commands. Not sure I really like that idea either as it seems like something that should be handled at the app level. That way the app can deal with potential errors as well. But at least it's just one connection property and hook to maintain rather than potentially N of them.

[cheenamalhotra]

I think main concern is that Date Format being one such factor that gets affected by locale and we modify it with specifying custom value in connection properties.

I would rather recommend something more generic, eg. "Regional" as we have in Microsoft ODBC Driver connection params, which when yes, uses client settings when converting currency, date, and time data to character data, while when no, would make driver use JDBC standard strings to represent currency, date, and time data that is converted to character data.

[gordthompson]

@cheenamalhotra

I understand your reasoning, but this patch is specific to the unusual handling of date strings for certain locales, and is specifically addressed by a T-SQL SET Statement (SET DATEFORMAT). The SET Statement document does not mention similar SET statements for currency or other data types that may have a variety of local string representations, so presumably they have not been sufficiently problematic for the core SQL Server team to address.

[peterbae]

Hi @gordthompson,

Sorry to have left this PR hanging here for a long time. We've reviewed this PR again, and based on the discussions it seems like we'd like a more generic approach to solving this problem, rather than introducing a connection property for each specific format. We've backlogged this item for now, and will return to this in the future.