Add support for variable precision TIMEs and DATETIMEs

[cgknx]

Q	A
Type	feature
Fixed issues	#2873

Summary

Adds support for high time precision times and datetimes where supported by the platform.

The feature is enabled by default for all platforms except SQLite and Oracle where the feature is opt-in (see below). Neither Oracle nor db2 can support high precision time columns without changing the column type used and so only high precision datetimes are supported on these platforms.

Use

To add high precision time to a column, add column['scale'] to the column specification to specify the number of decimal digits to include in the time. The maximum is platform specific. Since php's datetime objects hold a maximum of six decimal places, this is the effective practical maximum scale. Where fewer than six decimal digits are specified for a column, the time will be truncated or rounded depending on the platform.

Notes

PHP returns an error attempting to convert, for example, a time such as '10:10:10' (without decimal seconds) to a datetime object with a format of 'H:i:s.u' (with decimal seconds). To deal with columns specified as just DATETIME or DATETIME(0), there is a fallback attempt to convert using 'H:i:s' should 'H:i:s.u' fail.

SQLite

SQLite stores DATETIMEs as strings which can have arbitrary precision irrespective of column definition. This means a table could return any number of decimal places of precision for rows of one particular DATETIME column. This may be an issue because 10:10:10 != 10:10:10.0 as far as SQLite is concerned. Use of high precision times is therefore opt-in for SQLite.

To enable high precision datetimes and times in SQLite, add driverOption['high_precision_timestamps'] = true.

This driver option selects between the existing SQLitePlatform and a new SqliteHptPlatform which uses a high precision time format string.

Oracle

The format of datetimes returned by the database to DBAL is configured in driver middleware. The format does not include decimal digits. A driver configuration parameter has been added to change the underlying datetime format and enable high precision times.

To enable high precision datetimes and times in Oracle, add driverOption['high_precision_timestamps'] = true.

This driver option configures the middleware to add six decimal digits to NLS_TIMESTAMP_FORMAT and NLS_TIMESTAMP_TZ_FORMAT (i.e. .FF6) and selects a new OracleHptPlatform which uses high precision time format strings.

[derrabus]

Thank you for this PR. We might have handled precision on date fields a little inconsistently in the past. If we merge your PR, we should make sure we've taken all drivers into account and not just MySQL and Postgres.

SQL Server for instance also supports setting the precision on datetime2 fields. As far as I understand, we already set the precision to a value of 6 (although the default and maximum would be 7):

https://github.com/doctrine/dbal/blob/c982b9e16d35c028f4f03b4c421608a0c3e0f01d/src/Platforms/SQLServerPlatform.php#L1375

Oracle also supports a precision ranged from 0 to 9, but we hardcoded it to 0:

https://github.com/doctrine/dbal/blob/c982b9e16d35c028f4f03b4c421608a0c3e0f01d/src/Platforms/OraclePlatform.php#L322

DB2 supports a precision between 0 and 12 which we also hardcode to 0:

https://github.com/doctrine/dbal/blob/c982b9e16d35c028f4f03b4c421608a0c3e0f01d/src/Platforms/DB2Platform.php#L299

Can you look into how your changes would play with how we treat datetime columns on SQL Server and Oracle and DB2 currently? Unless I've missed something, this basically means that we don't need the supportsVariablePrecisionTime() override because all of the platforms are.

Another question: What would happen if we set the precision to a value greater than what the current platform supports, e.g. I set it to 9 and try to deploy that schema on a Postgres server?

[cgknx]

I'd seen the inconsistencies in terms of sometimes setting 0, 6 or no precision and took that into account with the patch set. I believe it deals with things like DATETIME2(6) and TIMESTAMP(0) correctly and sets the precision based on the length field in all cases although I don't have test systems to use. I can look at extending support to other databases relying on the CI checks to identify issues. I may need help debugging them though!

I used a supports flag so platforms can be added incrementally. If all have support then we can remove the supports flag and simplify the methods that depend on it as a final patch in the series (or a later patch).

In particular and as I noted in the PR, I'm not convinced SQLite's datetime precision support is robust and worry it will lead to issues. My understanding is that a particular column would store (string) datetimes from before the upgrade with no precision and those after the upgrade with microsecond precision. The inconsistency bothers me.

Finally, if we set datetime precision to 9 on a platform that doesn't support it, the maximum valid precision for the relevant platform would be used. No error would be generated so the schema you described would deploy albeit with a lower precision.

[cgknx]

I'll look at turning on support in other platforms later this week but hope I've addressed your first set of comments.

[derrabus]

I don't have test systems to use.

You can easily spin up each of the supported databases with docker. That's what our CI does as well.

[cgknx]

Support added for DB2, SQL Server and Oracle. Neither DB2 nor Oracle support high precision Time types because of the choice of column type and it is not obvious how to add such support in a BC manner (the commit message explains the detail).

Support not added for SQLite given potential BC issues already flagged and so supportsVariablePrecisionTime() is still used.

I'll squash the patchset down and rebase if necessary once you've had a chance to review.

[cgknx]

Additional patches added to reflect comments above. @derrabus, @morozov, let me know if you'd like me to squash them into the original patches before you take a look.

[cgknx]

Revised patchset with SQL declaration generation now reworked into lots of simpler methods. Additional functional tests added to test times and datetimes with timezone (these picked up an error in the existing Oracle platform which is fixed). Removed sanitization of $column['length'], instead leaving the db to generate the error (like excess precision with decimals).

[cgknx]

Oracle opt-in to high precision time now selected using a driver option to configure the middleware and platform selection by the driver (i.e. I'm proposing separate platforms for legacy and high precision times differing only in php datetime format strings). Same option used to add optional sqlite high precision time support so all platforms now supported.

[derrabus]

You PR contains a lot of unrelated commits now. Can you fix that? Otherwise, we cannot review it.

[cgknx]

Branch cleaned up now, I hope.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.

[jurchiks]

Why has this been seemingly abandoned? This feature has been needed for over 6 years now! All the tests pass, the only thing blocking this is people. @derrabus would you mind?

I just ran into this issue on a new project and it has been driving me insane for hours, and judging by the amount of comments on the original ticket, I am not even remotely the only one.

[derrabus]

Why has this been seemingly abandoned?

Because people have lives and families and day jobs, basically.

This feature has been needed for over 6 years now!

What's that supposed to tell me? This PR has been in progress since March this year which is when I had time to conduct the thorough review that this contribution needed. It was far from being mergable at that time.

All the tests pass, the only thing blocking this is people. @derrabus would you mind?

Yes, I do mind, actually. I really don't understand where this attitude is coming from. I'm not the only one able to test or review this contribution. I just happened to be the guy who conducted a review on March in his unpaid freetime.

I just ran into this issue on a new project and it has been driving me insane for hours, and judging by the amount of comments on the original ticket, I am not even remotely the only one.

So, you're the ideal volunteer to perform another review and test this feature on a real world project.

[jurchiks]

I really don't understand where this attitude is coming from. I'm not the only one able to test or review this contribution.

You are the only reviewer whose comments were pending at the time the PR went stale:

And since your comments were the pending ones, it makes sense that you would check up on them after the PR was updated.

Especially considering that the OP fixed what you asked on the same day you asked.

Because people have lives and families and day jobs, basically.

3 months...

[derrabus]

Sorry, but I'm not wasting any more energy on you. If you want to see this merged, make yourself useful. If you think you can bully me into working for free, get lost.

[jurchiks]

You asked for changes on this PR. Those changes were made the same day you asked. Are those changes as you wanted them to be or not? It's a simple question, and all I'm asking for is a yes or no on the PR.

[greg0ire]

And since your comments were the pending ones, it makes sense that you would check up on them after the PR was updated.

@jurchiks I think you have no idea what our inbox looks like. I have many, many notifications every day, to the point that I regularly mark all as read so as not to feel too overwhelmed. For Alexander, who is also a Symfony Core contributor, I guess it is way, way worse. Some of them come from people such as you that are "driven insane" by bugs, and do act insane as a consequence. I think you should calm down, try to understand the many reasons why what you did is very wrong, and then profusely apologize before attempting to ask any more questions.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.

[github-actions[bot]]

This pull request was closed due to inactivity.

[simPod]

@greg0ire @derrabus can we reopen? Where is it stuck? Is it waiting to being reviewed or is something still needed from the author?