Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to translate a database returned string to a requested System.DateTime #980

Closed
lauxjpn opened this issue Apr 29, 2021 · 9 comments

Comments

@lauxjpn
Copy link
Contributor

lauxjpn commented Apr 29, 2021

Check out the following SQL:

SELECT `g`.`Nickname`, COALESCE((
    SELECT `t1`.`IssueDate`
    FROM `Tags` AS `t1`
    WHERE `t1`.`GearNickName` = `g`.`FullName`
    ORDER BY `t1`.`Id`
    LIMIT 1), '0001-01-01 00:00:00') AS `invalidTagIssueDate`
FROM `Gears` AS `g`
LEFT JOIN `Tags` AS `t` ON (`g`.`Nickname` = `t`.`GearNickName`) AND (`g`.`SquadId` = `t`.`GearSquadId`)
WHERE `t`.`IssueDate` > COALESCE((
    SELECT `t0`.`IssueDate`
    FROM `Tags` AS `t0`
    WHERE `t0`.`GearNickName` = `g`.`FullName`
    ORDER BY `t0`.`Id`
    LIMIT 1), '0001-01-01 00:00:00')

It should return the following result set, with invalidTagIssueDate being of type datetime:

+------------+----------------------------+
| Nickname   | invalidTagIssueDate        |
+------------+----------------------------+
| Cole Train | 0001-01-01 00:00:00.000000 |
| Baird      | 0001-01-01 00:00:00.000000 |
| Dom        | 0001-01-01 00:00:00.000000 |
| Paduk      | 0001-01-01 00:00:00.000000 |
| Marcus     | 0001-01-01 00:00:00.000000 |
+------------+----------------------------+
5 rows in set (0.00 sec)

However, MySQL does not recognize, that the '0001-01-01 00:00:00' string is a datetime literal and just returns it as a string:

+------------+---------------------+
| Nickname   | invalidTagIssueDate |
+------------+---------------------+
| Cole Train | 0001-01-01 00:00:00 |
| Baird      | 0001-01-01 00:00:00 |
| Dom        | 0001-01-01 00:00:00 |
| Paduk      | 0001-01-01 00:00:00 |
| Marcus     | 0001-01-01 00:00:00 |
+------------+---------------------+
5 rows in set (0.00 sec)

The result is, that when a System.DateTime is requested from the data reader, MySqlConnector throws.

It would therefore make sense, that MySqlConnector gracefully parses a string to a DateTime, if a DateTime is requested by the user, but a string returned by the database.

Otherwise, Pomelo would need to explicitly cast all datetime literals explicitly to datetime, which is probably fine for us.
But fixing this MySQL behavior in MySqlConnector would also solve the issue for non Pomelo users.

@bgrainger
Copy link
Member

Are you asking for MySqlDataReader.GetDateTime to attempt to parse any string column (VARCHAR, etc.) as a ISO-8601 date/time and return it on success?

Otherwise, Pomelo would need to explicitly cast all datetime literals explicitly to datetime

It does seem like Pomelo's generated SQL should use CAST('0001-01-01 00:00:00' AS DATE) if the intention is to produce a DATE column; wouldn't this help with sorting, joins, etc.?

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Apr 29, 2021

Are you asking for MySqlDataReader.GetDateTime to attempt to parse any string column (VARCHAR, etc.) as a ISO-8601 date/time and return it on success?

Yes, that is basically it.

It does seem like Pomelo's generated SQL should use CAST('0001-01-01 00:00:00' AS DATE) if the intention is to produce a DATE column; wouldn't this help with sorting, joins, etc.?

Yes, that is what we will do, if MySqlDataReader.GetDateTime() is not going to implement the conversion itself. There is not really any need for us to cast in any other case as this one, where a datetime literal is just delegated to the SELECT statement, because in any other case (if used in a date function, join etc.), MySQL would implicitly cast the string to a datetime (as far as I know).

I just thought it might be worth to implement it in MySqlConnector, as it is not common for users to explicitly cast a datetime literal to a datetime, as MySQL usually just recognizes it without it, so it is likely that non-Pomelo users will run into this error as well, wondering why they are getting an InvalidCastException for a perfectly fine looking SQL query (of course they should have used parameters in the first place).

If you think that this should not be implemented in MySqlConnector, we will explicitly cast every datetime literal to a datetime, whether needed or not. This should not result in a performance hit, as it is done only once for constant literals and MySQL does it implicitly in all other cases anyway.

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Apr 29, 2021

Alright, so there are also Standard SQL prefixes available:

DATE 'str'
TIME 'str'
TIMESTAMP 'str'

MySQL uses the type keywords and the ODBC constructions to produce DATE, TIME, and DATETIME values, respectively, including a trailing fractional seconds part if specified. The TIMESTAMP syntax produces a DATETIME value in MySQL because DATETIME has a range that more closely corresponds to the standard SQL TIMESTAMP type, which has a year range from 0001 to 9999. (The MySQL TIMESTAMP year range is 1970 to 2038.)

So we can also just use them, which is probably more consistent with the upcoming .NET date and .NET time type support in EF Core.

@lauxjpn lauxjpn closed this as completed Apr 29, 2021
@bgrainger
Copy link
Member

I'm not opposed to implicit conversions in MySqlDataReader; we've added them in the past (e.g., #707, #664, #695, etc.).

I just want to have a better understanding of the pros and cons (adding tests to https://github.com/mysql-net/AdoNetApiTest would be a first step) before starting on it.

Immediately adopting DATE 'str' in Pomelo sounds good; I think this could certainly be solved with a both/and approach.

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Apr 30, 2021

So I think Pomelo will now definitely use the TIMESTAMP prefix, but it is probably still beneficial for other users to have an implicit conversion available to them. There is only little risk involved, as GetDateTime() would have thrown before. There should be no real impact on users.

The only slight impact I can see could be when sorting using this datetime literal (though it does not make a lot of sense to just sort by a constant datetime literal), which might sort as a string (though this would still be correct as the ISO string format sorts fine as a string, though other datetime formats might behave differently).

If there turn out to be inconsistencies after all, it might be better to not implement it at all, so that situations cannot end up more confusing than they would have been without it.


The only slight impact I can see could be when sorting using this datetime literal (though it does not make a lot of sense to just sort by a constant datetime literal), which might sort as a string (though this would still be correct as the ISO string format sorts fine as a string, though other datetime formats might behave differently).

Looks like this is not an issue (from 11.2 Date and Time Data Types):

Although MySQL tries to interpret values in several formats, date parts must always be given in year-month-day order (for example, '98-09-04'), rather than in the month-day-year or day-month-year orders commonly used elsewhere (for example, '09-04-98', '04-09-98'). To convert strings in other orders to year-month-day order, the STR_TO_DATE() function may be useful.

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Jun 25, 2021

We got another similar instance. Execute the following query under MySQL 8.0.25 and MySQL 5.7.34 against Northwind:

SELECT MIN(CASE
    WHEN 1 = `t`.`Key` THEN `t`.`OrderDate`
END) AS `Min`
FROM (
    SELECT `o`.`OrderID`, `o`.`OrderDate`, 1 AS `Key`
    FROM `Orders` AS `o`
) AS `t`
GROUP BY `t`.`Key`
ORDER BY `t`.`Key`

For MySQL 8.0.25, the returned type is a DateTime, which is expected.
For MySQL 5.7.34, the returned type is a string with a value of 1996-07-04 00:00:00 (unexpected), and so the GetDateTime() method throws.

While we could write some ugly workaround for this in Pomelo, it probably makes sense now to extend GetDateTime() to handle strings as well, since this issue seems to keep popping up.

@lauxjpn lauxjpn reopened this Jun 25, 2021
@bgrainger
Copy link
Member

with a value of 1996-07-04 00:00:00 (unexpected)

Does this mean that even if GetDateTime(n) returned a DateTime for this column, it would have the wrong value? (Would that cause the tests to fail anyway?)

Is the minimum functionality needed here to check for a string that's exactly 19 characters long and matches the pattern /^[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}$/, then treat it as an ISO date time and convert it to the equivalent DateTime (using the connection's DateTimeKind setting)?

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Jun 26, 2021

Does this mean that even if GetDateTime(n) returned a DateTime for this column, it would have the wrong value? (Would that cause the tests to fail anyway?)

No, the information carried is correct, just the type of the information (a string instead of a DateTime) is unexpected.

Is the minimum functionality needed here to check for a string that's exactly 19 characters long and matches the pattern /^[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}$/, then treat it as an ISO date time and convert it to the equivalent DateTime (using the connection's DateTimeKind setting)?

The store type could also contain a precision (currently up to 6), so the regular expression would look more like ^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d(?:\.\d{1,6})?$ (or yyyy-MM-dd HH\:mm\:ss.FFFFFF if you are using DateTime.TryParseExact()) but that is basically would need to be done.

@bgrainger
Copy link
Member

Added in 1.3.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants