HHH-18377 Support for uuid v6 and v7 generated ids #8719

cigaly · 2024-07-27T09:28:13Z

Implementation of UUID Versioin 6 and UUID Version 7

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

CaptainGlac1er · 2024-08-07T17:57:35Z

How will it handle if a separate server generates the same id into the same database? Will it recreate a new ID automatically and try again?

sebersole · 2024-09-17T17:02:09Z

How will it handle if a separate server generates the same id into the same database? Will it recreate a new ID automatically and try again?

No.

sebersole

I left a few comments about odd references to UUID#randomUUID(). Let's clean those up.

Additionally, you write great detail about what each value generator does. But you do it on #generateUuid. I'd prefer to see those on the class Javadoc, because...

Another thing we should do here is to add values for these into UuidGenerator.Style. Its probably best to have these link to the class Javadoc for the corresponding value-generator.

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidV6ValueGenerator.java

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidV7ValueGenerator.java

sebersole · 2024-09-19T12:26:01Z

A few naming related points here...

Not a fan of the enum names (UUIDV6, UUIDV7). How about VERSION6 and VERSION7 (or VERSION_6...)? Or since they are RFC compliant, RFC_6 and RFC_7?
The value generator names should use {Type}Strategy pattern to match with the others. E.g. Version6Strategy.

I'll need to pull this locally to squash the commits before I apply. I can apply the name changes at that time if you prefer.

Thanks again for the work on this!

cigaly · 2024-09-19T13:16:06Z

A few naming related points here...

1. Not a fan of the enum names (UUIDV6, UUIDV7).  How about VERSION6 and VERSION7 (or VERSION_6...)?  Or since they are RFC compliant, RFC_6 and RFC_7?

2. The value generator names should use `{Type}Strategy` pattern to match with the others.  E.g. `Version6Strategy`.

Changed both things.

I'll need to pull this locally to squash the commits before I apply. I can apply the name changes at that time if you prefer.

I've squashed everything into two commits, one for implementation, other for test case. Hope that this is OK?

Thanks again for the work on this!

You're welcome. Happy if I can help. Not that I was too busy lately.

sebersole · 2024-09-19T13:19:01Z

In general, we like the test/impl split for the PR as it shows (1) what is addressed and (2) how it was addressed.

When we apply them, though, we generally (not always) squash to one as it makes it easier to cherry-pick to other branches. This we won't be backporting, so its less important. Long story short, 2 commits is fine here.

CaptainGlac1er · 2024-09-19T13:25:12Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidGenerator.java

+			else if ( config.style() == VERSION_6 ) {
+				return new UuidVersion6Strategy();
+			}
+			else if ( config.style() == VERSION_7 ) {


Would a switch case make sense?

You certainly could. Personally I don't see the benefit here.

Not sure if this will look any better than now ...

if ( config.algorithm() != UuidValueGenerator.class ) { ... } else { switch (config.style()) { ... } }

You won't need the else since the first if statement returns.

I think that is true, and was actually my fault from the first :)

It really could/should be:

if ( config != null ) { if ( config.algorithm() != UuidValueGenerator.class ) { // this block returns or throws an exception... } if ( config.style() == TIME ) { return ... } if ( config.style() == VERSION_6 ) { return UuidVersion6Strategy.INSTANCE; } if ( config.style() == VERSION_7 ) { return UuidVersion7Strategy.INSTANCE; } } return StandardRandomStrategy.INSTANCE;

Or like

if(config == null) { return StandardRandomStrategy.INSTANCE; } if(config.algorithm() != UuidValueGenerator.class) { // throw or return logic } switch(config.style()) { case TIME: return .. case VERSION_6: return UuidVersion6Strategy.INSTANCE; case VERSION_7: return UuidVersion7Strategy.INSTANCE; }

Well no. You are missing what is effectively the AUTO, which also happens to be the config == null case.

I'm not following the desire to add a switch just to add a switch. A switch with so few case options is unuseful imo. In fact, IntelliJ will even suggest this as a refactoring.

Well was trying to flatten the if statement and moved it up as a guard clause for the AUTO.
https://blog.codinghorror.com/flattening-arrow-code/

CaptainGlac1er · 2024-09-19T13:28:06Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion7Strategy.java

+		final long nanosPart = Math.round( ( currentTimestamp.getNano() % 1_000_000L ) * 0.004096 );
+
+		return new UUID(
+				millis << 16 & 0xFFFF_FFFF_FFFF_0000L | 0x7000L | nanosPart & 0xFFFL,


Feel fine to ignore this comment, but would it make it more self documenting to save this mask to a constant?

wdyt @cigaly ?

Or at least some code comments?

I would advise against code comments, as code comments tend to trend to lie as the project ages.
https://dev.to/ruben_alapont/magic-numbers-and-magic-strings-its-time-to-talk-about-it-ci2

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidGenerator.java

sebersole

A few minor suggestions/requests. Let me know what you think. FWIW I left "suggested changes" in those places to make it easy to apply if you agree.

sebersole · 2024-09-19T13:38:39Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion6Strategy.java

+ *     <li>14 bits - the clock sequence, resets to 0 when timestamp changes. </li>
+ *     <li>48 bits - pseudorandom data to provide uniqueness.</li>
+ * </ul>
+ *


Suggested change

*

*

* @apiNote Version 6 is field-compatible with Version 1, with the time bits reordered for improved DB locality.

*

sebersole · 2024-09-19T13:53:14Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion6Strategy.java

+	 */
+	@Override
+	public int getGeneratedVersion() {
+		// UUIDv6 is a field-compatible version of UUIDv1, reordered for improved DB locality


Suggested change

// UUIDv6 is a field-compatible version of UUIDv1, reordered for improved DB locality

sebersole · 2024-09-19T13:56:01Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion6Strategy.java

+	 * @param session session
+	 *
+	 * @return UUID version 6
+	 * @see UuidValueGenerator#generateUuid(SharedSessionContractImplementor)


Suggested change

* @param session session

*

* @return UUID version 6

* @see UuidValueGenerator#generateUuid(SharedSessionContractImplementor)

* Generates a Version 6 compliant {@linkplain UUID} value.

*

* @return A Version 6 compliant {@linkplain UUID}.

*

* @see UuidValueGenerator#generateUuid

sebersole · 2024-09-19T14:00:36Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion7Strategy.java

+ *     <li>14 bits - counter to guarantee additional monotonicity, resets to 0 when timestamp changes. </li>
+ *     <li>48 bits - pseudorandom data to provide uniqueness.</li>
+ * </ul>
+ *


Suggested change

*

*

* @apiNote A Version 7 UUID features a time-ordered value field derived from the widely

* implemented and well known Unix Epoch timestamp source, the number of milliseconds

* since midnight 1 Jan 1970 UTC, leap seconds excluded.

*

sebersole · 2024-09-19T14:01:34Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion7Strategy.java

+		/*
+		 *	UUIDv7 features a time-ordered value field derived from the widely implemented and well-
+		 * known Unix Epoch timestamp source, the number of milliseconds since midnight 1 Jan 1970 UTC,
+		 * leap seconds excluded.
+		 */


Suggested change

/*

* UUIDv7 features a time-ordered value field derived from the widely implemented and well-

* known Unix Epoch timestamp source, the number of milliseconds since midnight 1 Jan 1970 UTC,

* leap seconds excluded.

*/

sebersole · 2024-09-19T14:02:44Z

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidVersion7Strategy.java

+	 * @param session session
+	 *
+	 * @return UUID version 7
+	 * @see UuidValueGenerator#generateUuid(SharedSessionContractImplementor)


Suggested change

* @param session session

*

* @return UUID version 7

* @see UuidValueGenerator#generateUuid(SharedSessionContractImplementor)

* Generates a Version 7 {@linkplain UUID}.

*

* @return A Version version 7 {@linkplain UUID}.

* @see UuidValueGenerator#generateUuid(SharedSessionContractImplementor)

sebersole · 2024-09-19T14:19:58Z

Also, clearly we need to address the failures :)

Execution failed for task ':hibernate-core:compileTestJava'.
> Compilation failed; see the compiler error output for details.

cigaly · 2024-09-19T14:43:39Z

Also, clearly we need to address the failures :)

Execution failed for task ':hibernate-core:compileTestJava'.
> Compilation failed; see the compiler error output for details.

One will expect that IDE should know to make basic refactoring things like renaming classes :-)

…ersion 7 UUID's

cigaly force-pushed the HHH-18377 branch 3 times, most recently from 5b0017a to 066a676 Compare July 29, 2024 15:28

cigaly marked this pull request as ready for review July 29, 2024 16:21

cigaly force-pushed the HHH-18377 branch from 066a676 to 182b8e0 Compare July 29, 2024 16:21

cigaly force-pushed the HHH-18377 branch from 182b8e0 to 08435f6 Compare August 9, 2024 16:47

sebersole requested changes Sep 17, 2024

View reviewed changes

cigaly force-pushed the HHH-18377 branch 2 times, most recently from 5134d12 to e44c59b Compare September 19, 2024 05:43

cigaly force-pushed the HHH-18377 branch from e44c59b to ba08ad0 Compare September 19, 2024 13:11

sebersole approved these changes Sep 19, 2024

View reviewed changes

CaptainGlac1er reviewed Sep 19, 2024

View reviewed changes

sebersole reviewed Sep 19, 2024

View reviewed changes

hibernate-core/src/main/java/org/hibernate/id/uuid/UuidGenerator.java Show resolved Hide resolved

cigaly force-pushed the HHH-18377 branch from ba08ad0 to 20467ee Compare September 19, 2024 13:56

sebersole reviewed Sep 19, 2024

View reviewed changes

cigaly added 2 commits September 19, 2024 17:02

HHH-18377 UUID Version 6 & UUID Version 7 implementations

b57e84e

HHH-18377 Test cases to check monofonicity of generated version 6 & v…

90e4f06

…ersion 7 UUID's

cigaly force-pushed the HHH-18377 branch from 20467ee to 90e4f06 Compare September 19, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HHH-18377 Support for uuid v6 and v7 generated ids #8719

HHH-18377 Support for uuid v6 and v7 generated ids #8719

cigaly commented Jul 27, 2024 •

edited by jira bot

Loading

CaptainGlac1er commented Aug 7, 2024

sebersole commented Sep 17, 2024

sebersole left a comment

sebersole commented Sep 19, 2024

cigaly commented Sep 19, 2024

sebersole commented Sep 19, 2024 •

edited

Loading

CaptainGlac1er Sep 19, 2024

sebersole Sep 19, 2024

cigaly Sep 19, 2024

CaptainGlac1er Sep 19, 2024

sebersole Sep 19, 2024

CaptainGlac1er Sep 20, 2024

sebersole Sep 20, 2024

CaptainGlac1er Sep 20, 2024

CaptainGlac1er Sep 19, 2024

sebersole Sep 19, 2024

CaptainGlac1er Sep 19, 2024

sebersole left a comment

sebersole Sep 19, 2024

sebersole Sep 19, 2024

sebersole Sep 19, 2024

sebersole Sep 19, 2024

sebersole Sep 19, 2024

sebersole Sep 19, 2024

sebersole commented Sep 19, 2024

cigaly commented Sep 19, 2024

- *
+ *
+ * @apiNote Version 6 is field-compatible with Version 1, with the time bits reordered for improved DB locality.
+ *

- *
+ *
+ * @apiNote A Version 7 UUID features a time-ordered value field derived from the widely
+ * implemented and well known Unix Epoch timestamp source, the number of milliseconds
+ * since midnight 1 Jan 1970 UTC, leap seconds excluded.
+ *

HHH-18377 Support for uuid v6 and v7 generated ids #8719

Are you sure you want to change the base?

HHH-18377 Support for uuid v6 and v7 generated ids #8719

Conversation

cigaly commented Jul 27, 2024 • edited by jira bot Loading

CaptainGlac1er commented Aug 7, 2024

sebersole commented Sep 17, 2024

sebersole left a comment

Choose a reason for hiding this comment

sebersole commented Sep 19, 2024

cigaly commented Sep 19, 2024

sebersole commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebersole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebersole commented Sep 19, 2024

cigaly commented Sep 19, 2024

cigaly commented Jul 27, 2024 •

edited by jira bot

Loading

sebersole commented Sep 19, 2024 •

edited

Loading