-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
MDEV-33710 Implement UUID_TIMESTAMP() function #4496
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
base: main
Are you sure you want to change the base?
MDEV-33710 Implement UUID_TIMESTAMP() function #4496
Conversation
…v1 and UUIDv7 values
5bbed91 to
9aa596f
Compare
|
I have some concern about the tests... it's not easy to validate the UUID values used there.
We can test a couple of random values, plus corner cases: min and max value for timestamp variable, and for UUID |
|
Strangely, there is a different result outpoutted in amd64-debian-11-debug-ps-embedded test run: Please take a look |
|
i think the test failing is a timezone issue (exact 5.30 HRS), ill fix that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @varundeepsaini for the update, and besides such aquick one!
The PR moves in the right direction. Still some work to be done.
plugin/type_uuid/mysql-test/type_uuid/func_uuid_timestamp.result
Outdated
Show resolved
Hide resolved
plugin/type_uuid/item_uuidfunc.cc
Outdated
|
|
||
| my_time_t seconds; | ||
| ulong usec; | ||
| const uchar *buf= (const uchar *) uuid.to_lex_cstring().str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functions really need uchar (except the newly added one, which we can change), also uuid is natively stored in char, so why make a conversion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mi_ prefix functions use uchar (they do cast it themselves tho, i can remove it from here but again, the cast is still there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean? where??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SELECT UUID_TIMESTAMP('1ee81f00-a8a8-11ee-8000-000000000000') IS NOT NULL AS has_timestamp; | ||
|
|
||
| # Test dynamic UUID generation - timestamp should match NOW() | ||
| SELECT TIMESTAMPDIFF(SECOND, UUID_TIMESTAMP(UUID()), NOW()) AS v1_diff_seconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is (sort of, implicitly) indeterministic: each time it will check against the new NOW value. So if it will ever fail (because of the bug), noone will be able to reproduce it.
Fix NOW() with set timestamp statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UUID generation uses my_interval_timer() which gets the real system time, not the session's SET timestamp
i looked at other tests as well and tey also use this pattern itself (with NOW() / CURDATE() )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then using it vs NOW etc would mean test would fail sporadically. Can you show the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides we had no timestamp extraction before, so what do you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i grepped NOW() / CURDATE() and found these tests
hence did something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can go the other way:
Generate some UUID, somewhere, for example https://www.uuidgenerator.net/, or right in mariadb
Decode: https://www.uuidtools.com/decode
For example, 2731d872-e26e-11f0-8de9-0242ac120002 has 2025-12-26 15:18:38.034341.0 UTC
add test that outputs both correct timestamp (from decoder) and the result from your function
Alternatively, we could add a mockup for using timestamp in my_uuid, but maybe even better if we will do it in the less invasive way.
mysys/my_uuid.c
Outdated
| Bytes 6-7: version (4 bits) + sub-millisecond precision (12 bits) | ||
| */ | ||
|
|
||
| my_bool my_uuid_extract_ts(const uchar *uuid, my_time_t *seconds, ulong *usec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use just int.
Or alternatively, bool, with true meaning success
|
Never mind the failing appveyor, it is used to be failing. However it may be useful to check the results, as it can reveal some windows build quirks (for example msvc doesn't like bool vs int mismatch, and has a 32-bit |
Description
Adds
UUID_TIMESTAMP(uuid)function to extract timestamps from UUIDv1 and UUIDv7 values. ReturnsNULLfor other versions (e.g., v4) or invalid input.Release Notes
New function:
UUID_TIMESTAMP(uuid)- Returns the generation timestamp from UUIDv1/v7 asTIMESTAMP(6).How can this PR be tested?
Basing the PR against the correct MariaDB version
mainbranch.PR quality check