feat: implement FromCqlVal and SerializeValue for PrimitiveDatetime #1115
Conversation
|
See the following report for details: cargo semver-checks output |
|
Please note that |
|
Done @wprzytula |
github-actions
bot
added
the
semver-checks-breaking
|
Could you add relevant unit tests, too? Just to be sure the implementation is correct. |
Do we need test cases for it though? It uses already existing implementation of Serialization and deserialization |
I agree that the ser/de part of this PR is minimal. However, we are still adding a new type that wasn't tested in any way before and it's a good habit to have at least one test: ser/de equivalence, on one concrete value of the type. |
wprzytula
changed the title
|
@wprzytula Sure added the test you can check, I can see |
|
When working on my implementation, I referenced this doc a lot What it says about timestamps without timezone is that they're assumed to be in the local timezone of the coordinating ScyllaDB node, while the implementation in this PR uses UTC. My hesitation is that it may result in confusing behavior if some other library is used for a different server, for example. The doc also recommends always explicitly adding timestamps to avoid this exact potential ambiguity in implementations, as far as I understand. |
|
I can use the |
|
I'm not a frequent contributor, I just happened to make that PR, so please correct me if I'm wrong. The timezone on a coordinating ScyllaDB node may not match the timezone on the machine where this current code is executed, so the Unfortunately, I myself am not sure how to address this problem and if it's a major problem at all, or just a niche case that should be documented. I'd prefer some input from other contributors on that matter. |
|
I don't think it's ever possible for a client to know the server's system time. I think when they say local time they must mean the client local time. Because if the client is providing the CQLTimestamp input to the database the client must be providing the client's local time. I am also not sure how the server would get the local time, Let's say if I have a table and I have default value set on my table schema for a field with type Timestamp. Every time I insert a data the server would take local time and if the nodes are in different regions this local time would vary everywhere. So I am not entirely sure. |
|
Hey @Anfid @wprzytula , I also checked the TryInto implementation from OffsetDatetime to CqlTimeStamp, You are using the function |
|
@dracarys18 This is reasonable for unix timestamp, as it's specifically defined to be in UTC, and then you get local time by adding timezone offset information to it. |
|
@Anfid Correct me if I am wrong, that's the same behavior followed in this PR right? |
|
@dracarys18 not really, as |
|
In general, you can break timestamps into the following categories:
|
|
@Anfid I agree that PrimitiveDateTime wont have the timezone context, Here for this use case we are converting a |
|
The point of my message above is precisely about how I don't consider implicit conversion of To illustrate a potential example of problems that it might cause, let's think about some hypothetical calendar or reminders app. When user creates an event, it contains date and time info, but no timezone. If the developer forgets to add timezone info manually and writes a An even more problematic example might occur of there's a mix of events serialized from Ideally I personally prefer types to support the development process as much as possible. However if others have different opinions on that matter, I wouldn't start a war over it :) |
|
@wprzytula @Lorak-mmk Can we please merge this? |
The doc describes textual format, used e.g. when querying manually through clqsh. Binary cql format uses unix epoch as a reference point, and thus utc:
This is true, but not really relevant because of what I wrote above.
The docs do mean coordinator local time, and not client local time. In the case you are discussing, server gets a string with time, without timezone. Server doesn't know client's timezone, so it uses its own. Why it uses coordinator local time instead of utc? No idea, probably C* did it that way so Scylla had to follow.
The comments from @Anfid ( #1115 (comment) #1115 (comment) #1115 (comment) ) convinced me that we should not add this implementation. |
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.