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

Sqlservertime #1872

Merged
merged 1 commit into from Apr 18, 2018

Conversation

Projects
None yet
3 participants
@smootoo
Copy link
Contributor

commented Mar 13, 2018

No description provided.

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

As requested. A new PR based on #1825
Don't merge this one yet. I'll fix the Oracle tests first.

BTW, do we still care about the CircleCI build. It looks like it's failing on master as well. I could fix that too?

@hvesalai

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

I must say I don't know when the circleci tests are run. I have not been following them.

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

Just a quick update on where I am with this. I got the Oracle profile working with the existing tests, but as I was reviewing the code in the profile, I was confused as to how it could be working. Turns out it didn’t work :-) There is a gap in the test coverage. In particular around updating ResultSets and what’s happening at DST change overs. I’m increasing the test coverage and it’s highlighted some issues with the other profiles as well. I’ll work through those and update this PR. I should have something by this weekend.

@hvesalai hvesalai added this to the 3.3 milestone Mar 26, 2018

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

Another quick update on where I am. The new tests I added highlighted a few issues, so I've been working through them. It's taken a while, but I'm getting close now. Only the DB2 and postgres tests to fix. Hopefully have them done and the code tidied up in a few days time.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

Thank you for the update. Good progress!

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

Almost there now. This has turned into a bit of an epic! Fixing the postgres issues were particularly tricksy.
All the builds are green now though. I'll re-review the code in its entirety and beef up some of the comments as I've been heads-down fixing one issue after another.

@@ -180,6 +185,70 @@ trait DB2Profile extends JdbcProfile {
override def sqlTypeName(sym: Option[FieldSymbol]) = "CHAR(1)"
override def valueToSQLLiteral(value: Boolean) = if(value) "1" else "0"
}

class InstantJdbcType extends super.InstantJdbcType {
override def sqlTypeName(sym: Option[FieldSymbol]) = "TIMESTAMP(6) WITH TIME ZONE"

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 6, 2018

Member

Why would Instant have a time zone? Instant is just time steps since 1970-01-01T00:00:00Z, i.e. it is always Zulu time

@hvesalai
Copy link
Member

left a comment

There is still some design and policy needed in the parsing of Instant. Let's discuss and then do the required changes.

The first thing to decide is, what kind of a field to define for instant when a) there is native support for sub-second instants or b) there is no support for them.

If I understand the code correctly, currently for b) a string is used, which I don't think is so wise.

The second thing to decide is whether or not to store the timezone for instants. I think it should not be stored because instants should always be in UTC (i.e. Z, +0000, etc). This should be implied.

The third thing to discuss is reading Instants. So even though we write instants in some way it doesn't mean the database has values only in that format. In my opinion reading should be lenient so that, for example, if there are values in some other timezone than UTC, then those are still read and converted correctly acording to the timezone.

override def setValue(v: Instant, p: PreparedStatement, idx: Int) : Unit = {
p.setString(idx, if (v == null) null else v.toString)
}
override def getValue(r: ResultSet, idx: Int) : Instant = {

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 6, 2018

Member

this is different from the DerbyProfile, which uses Instant.parse Why are they different?

r.updateString(idx, v.toString)
}
override def valueToSQLLiteral(value: Instant) = {
s"'$value'"

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 6, 2018

Member

please add .toString

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

So use the same format as in all the other DBs

implicit val max : Instant = Instant.MAX
implicit val serializeFiniteTime : (Instant => String) = _.toString
implicit val parseFiniteTime : (String => Instant) = {
LocalDateTime.parse(_, formatter).toInstant(ZoneOffset.UTC)

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 6, 2018

Member

Is this really safe? Here you convert the string from the database to the Local timezone and then to Instant. As I understand it, you would get wrong time if the system timezone is not UTC and the time didn't contain a timezone (which would be OK for instant since it is implicitly always in UTC)

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 6, 2018

Author Contributor

LocalDateTime doesn't apply the local timezone, it's a data type that doesn't have a timezone.
And since Timestamps in Postgres are UTC, then the system timezone doesn't play any part here.

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

I wrote this as you were making more comments. In summary though, I think the timestamp is mostly the wrong datatype for Instants as DST shifts may be applied to it depending on configuration.

The timestamp (without time zone) database type is handled different in different db backends (and jdbc implementations). Postgres for example stores timestamps as UTC and so they are the datatype for Instants.
Depending on timezone configurations in the database backend and on the client JVM, DST may be applied to the timestamp. In a timezone where DST is in effect, then an hour may be gained or lost during the changeover. For example, in ‘London/Europe’, if you try to write the instant ‘2018-03-25T01:37:40Z’ into a timestamp column, it will come back from the db as ‘2018-03-25T02:37:40Z’.
This is acceptable and probably intended for LocalDateTime, but Instants need to be continuous and always increasing, with a larger value being later on the timeline.
Where timestamps with time zone are supported by the db backend, then writing Instants as timestamps with timezone ‘UTC’ or offset ‘+00:00’ gives us the characteristics we need.

I’ll write up something like the above in a big comment so that people know what’s going on and why.

Reviewing this PR is the time to pick up on this though, so thanks for looking. We now have a comprehensive set of tests try alternative implementations to make sure there are no edge cases where values aren’t round-tripped correctly.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Ok, so let's write the Z or +0000 time offset to the database. But I don't still think we can accept 2018-03-25T01:37:40Z becoming 2018-03-25T02:37:40Z. Why would that be acceptable?

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

I agree with you that for Instants, 2018-03-25T01:37:40Z must stay unchanged whatever the locally configured time zone is.
But if that same Instant is converted into a LocalDateTime (i.e. a type without a timezone) and that is stored in a database that is configured with a local time zone of London/Europe into a database column of type TIMESTAMP then because this time is in the local DST gap (the clocks have gone forward an hour), the db will turn that time into 2018-03-25T02:37:40. This is what I meant by probably intended behaviour for LocalDateTime, that any locally enforced DST gaps are applied, but the value still doesn't have a timezone.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Ah ok, so then we need just big block letters saying Caveat emptor DST

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

Indeed. There are dragons here everywhere! Like I said, I'll stick in a big comment explaining what's going on. I personally couldn't think of a use for LocalDateTime that isn't better served with another time type. But I guess it supports some legacy behaviours.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Instead of a big comment, should we invest in a new page on the documentation?

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

Sure. I'll draft something at the weekend. OK?

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Yes, that sounds good!

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Also, if you can say when the code is ready for a new round of code-review, I can then review and add comments.

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

Sorry I updated this later than I had planned. Real life got in the way over the last week!

I added a page of documentation. The code is ready for review now.

The PR has been open in various forms for a long time and has had many different contributors. A fresh pair for eyes would good now. Appreciate any comments you may have.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@smootoo can you rebase your PR, there is some conflicts

That means that you get back the same value that was stored, irrespective of the configuration of the
database or the JVM.

2. It should be clear to you as a user of the database what those values are. This may be with an appropriate database type, if one is available. If not, a varchar with a "stringified" value will be used.

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

stringIfield? what does the I stand for?

Is the string value such that it will collate properly, i.e. <, > = work as expected?

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

Removed my made up word.
Elaborated on the comparability of the string values.


The different java.time types have important characteristics. E.g `Instant`s need to be continuous and always increasing,
with a larger value being later on the timeline.
The timestamp (without time zone) database type is handled different in different db backends (and jdbc implementations). Postgres, for example, stores timestamps as UTC and so they are approproriate to be the datatype for Instants. Depending on timezone configurations in the database backend and on the client JVM, DST may be applied

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

Re the part starting "Depending on": does it pertain to psql, or do you mean that "For other dataabases, depending on..."

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

Explain DST, i.e. "DST (Daylight saving time)"

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

I did mean for db others than postgres. Updated.
Added explanation for DST and why it's a problem.

One java.time type to be careful of is `LocalDateTime`, that behaves most like the legacy `java.sql.Timestamp` and is mapped into a Timestamp database type. For these, the exact behaviour for DST related can depend on the environment configuration of the database or the JVM. And so is the one type that can break the "roundtripping" rule if the value being stored falls into a DST gap for a given environment configuration.

For example, if your environment is configured to be `Europe/London`, and you try to write the `LocalDateTime` `2018-03-25T01:37:40` into a database that performs DST shifts, such as Oracle, it will come back from the db as `2018-03-25T02:37:40`. This may be the desired behaviour, and is what happens with some databases for a Timestamp without timezone type. If this isn't the desired behaviour, then using a more appropriate type, e.g. `Instant` or `ZonedDateTime` will not have a DST gap issues, irrespective of environment configuration.

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

Could we have one more paragraph on how to use your own mapper if the default ones are not appropriate?

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

This isn't really specific to the data and time types. There is already stuff here (doc/src/schemas.md). Do you want me to add to that?

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 17, 2018

Member

No, I think it is sufficient to just state the fact that if you are not happy with the default mapping, you can do your own, see [link to schmas.md] on how to do that.

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 17, 2018

Author Contributor

Documentation updated

@@ -79,7 +79,7 @@ class Tables(val profile: JdbcProfile){
class X(tag: Tag) extends Table[(Int,Int,Option[Int],Int,Double,String,Option[Int],Option[Int],Option[String],Option[String],Option[String])](tag, "X") {
def pk = column[Int]("pk")
def pk2 = column[Int]("pk2")
def pkpk = primaryKey( "", (pk,pk2) ) // pk column collision
def pkpk = primaryKey( "primary_key2", (pk,pk2) ) // pk column collision

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

What is this change? Should it be a separate PR?

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

Increasing the H2 version to 1.4.197 (which was done to get timestamp with timezone support), changed its handling of un-named db artifacts. They get now get a name of null and so if there is more than one un-named db artifact, there will be a name clash. This change gives the primary key a name. The H2 jdbc library change and this fix to the test could be done as a separate PR.

This comment has been minimized.

Copy link
@hvesalai

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 17, 2018

Member

I would like it to be a separate commit. If I do a squash merge for this PR, then that is lost. Two alternatives:

  • you make the squash of this PR before merging but leave this change to a separate commit in the PR that has a commit message explaining the above; or
  • a new PR with just this change (and a commit message explaining the above)

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 17, 2018

Author Contributor

PR #1891 has been raised

r.updateString(idx, v.toString)
}
override def valueToSQLLiteral(value: Instant) = {
s"'$value'"

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

So use the same format as in all the other DBs

// not adding trailing zeros to the hours. For example: '2:14:41.421' instead of '02:14:41.421'
r.getString(idx) match {
case null => null
case isoTime if isoTime.indexOf(':') == 2 => LocalTime.parse(isoTime)

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

Why is the formatter on line 162 not used?

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

The formatter on line 162 is for offset times. It has a non-optional offset component and so can't be used for LocalTime. The default LocalTime parser (which is used here) works fine though.

/**
* Add offset padding e.g. +1:00 becomes +01:00.
*/
def offsetConvertHsqldbToISO(hsqldbString : String) : String = hsqldbString.takeRight(5).toArray match {

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

Why are these needed? They look a bit suspicious to me. Couldn't you just use a DateTimeFormatter instead?

Please add a comment on why a DateTimeFormatter cannot be used.

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

The comment on lines 157 to 159 explain why these are needed in addition to a DateTimeFormatter This whole trait is for handling that the offsets don't have leading zeros.

This comment has been minimized.

Copy link
@hvesalai
// So, store them at UTC timestamps, otherwise the JDBC layer might attempt to map them
// and with DST changes, there are some times which will be unrepresentable during the switchover
class LocalDateTimeJdbcType extends super.LocalDateTimeJdbcType {
// private[this] val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS x")

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

remove the commented out code if there is no reason to keep it. If there is a reason, document it.

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

Commented out code removed

@@ -404,3 +556,180 @@ object OracleProfile extends OracleProfile {
case class AutoIncTriggerName(name: String) extends ColumnOption[Nothing]
}
}


/* The following code will be used when the native implementation of

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

what does this comment mean? "native implementation of ... are natively implemented"

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

That comment was left over from one of the intermediate commits. Removed.

}
override def valueToSQLLiteral(value: Instant) = {
value match {
case null =>

This comment has been minimized.

Copy link
@hvesalai

hvesalai Apr 16, 2018

Member

this is the only profile that handles value == null. Why don't the other do it?

This comment has been minimized.

Copy link
@smootoo

smootoo Apr 16, 2018

Author Contributor

The case null => couldn't be reached. They've been removed.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Ok. Now there is a plethora of comments. Can you go through them all and reply / make changes as needed.

Also it would be very good if @tminglei had time to read the Postgres code here.

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

@hvesalai I've updated based on your comments. Thanks.
I don't think this needs a rebase. I can merge as is, cleanly.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

Added two more requests to review coments.

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

Almost there. Now if you can squash all the commits to one and rebase, then I can hit the merge button. Now this PR still contains the same changes as in #1891, but after a rebase they should be gone.

@smootoo smootoo force-pushed the smootoo:sqlservertime branch from b342cf3 to 01b6b03 Apr 18, 2018

@smootoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

I've squashed all the commits and rebased. Hopefully this is it :-)

@hvesalai hvesalai merged commit 1282f18 into slick:master Apr 18, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

Here goes the planet! Thank you!

@hvesalai

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@nafg

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Well... this broke my app, and not at compile time...

I have different columns of different SQL types in various places that are mapped to Instant. I don't see a workaround other than doing one of the following (which I probably should do anyway, but that doesn't mean I would prefer less overhead right now):

  1. Convert the old column types
  2. Use something like a tagged type, so that the column type includes the underlying type somehow but values can still be used as the mapped type (essentially along the lines of Instant with Phantom[Long])
@hvesalai

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@nafg there was somebody with identical problem I think. Just can't remember where that discussion is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.