WIP feat(idgen): Start Implementation of NoSQL with the ID Generation Framework #2131

adam-christian-software · 2025-07-18T01:47:39Z

This PR is still in draft.

Motivation

This implementation starts the series of PRs to implement the NoSQL work presented in https://docs.google.com/document/d/1POUWe0xMZOBoaJ6Rgiw35ziEoc6OEYCiW7Zk6bR9H6M/edit?tab=t.0#heading=h.nx9vzhg2x8v2 with the first implementation here.

Description

This PR is dedicated to creating the ID Generation Framework.

Related to #650 & #844

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/IdGenerator.java

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/MonotonicClock.java

dimas-b · 2025-07-18T02:41:01Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/SnowflakeIdGenerator.java

+
+public interface SnowflakeIdGenerator extends IdGenerator {
+  /** Offset of the snowflake ID generator since the 1970-01-01T00:00:00Z epoch instant. */
+  Instant EPOCH_OFFSET =


nit: this is not technically an offset, but an exact moment in time. How about just EPOCH?

Also, the expression is unnecessarily complex, the below one is 100% equivalent and imho easier to grasp:

Instant.parse("2025-03-01T00:00:00Z")

@dimas-b - At the risk of sounding too verbose, how about ID_GENERATOR_EPOCH? I'd like to distinguish this from Unix Epoch?

And, I agree @adutra . I'll update.

ID_GENERATOR_EPOCH LGTM. Alt: ID_EPOCH.

I like ID_EPOCH. I'll do that.

persistence/nosql/idgen/spi/src/main/java/org/apache/polaris/ids/spi/IdGeneratorFactory.java

.../nosql/idgen/impl/src/main/java/org/apache/polaris/ids/impl/SnowflakeIdGeneratorFactory.java

persistence/nosql/idgen/impl/src/main/resources/META-INF/beans.xml

dimas-b · 2025-07-18T03:07:53Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/IdGenerator.java

+  long generateId();
+
+  /** Generate the system ID for a node, solely used by/for node management purposes. */
+  long systemIdForNode(int nodeId);


Is this ID expected to be the same for all IdGenerator implementations? What if a future impl. is not node-based?

Should this be pushed down to the Snowflake ID code?

This function isn't specific to the particular implementation.
See the upcoming node-id-lease stuff: there is one implementation with one constant configuration per setup.

Actually, the more that I read about the node-id lease stuff. I don't know whether this should be in this module. Here's my thinking:

It seems that NodeID generation is special and not a Snowflake ID generation. The implementation is only based upon the node id passed in and some system-wide variables such as SnowflakeIdGeneratorImpl#timestampMax, SnowflakeIdGeneratorImpl#timestampShift, & SnowflakeIdGeneratorImpl#sequenceBits. So, in practice, it's really just the Node ID passed in.

So, given that, I think we could pull this out and just put it into the node leasing modules. That way, we can keep the IdGenerator clean for the cases that require the distributed id generation.

What do y'all think?

+1 to Adam's point 1.

... however, I have a bigger concern. Suppose we run with Snowflake IDs for a while and then change to another ID generator. Assume generateId() outputs do not clash. Still, do we expect systemIdForNode(X) to return the same value for all generator implementations and for all possible values of X?

adutra · 2025-07-18T09:31:07Z

persistence/nosql/idgen/spi/build.gradle.kts

+  compileOnly(platform(libs.quarkus.bom))
+  compileOnly("io.quarkus:quarkus-core")
+
+  compileOnly(project(":polaris-immutables"))


This dependency seems unused. I wonder if you didn't forget to annotate IdGeneratorSource with @PolarisImmutable? It seems like a good candidate for that (lots of anonymous classes implementing this interface).

adutra · 2025-07-18T09:33:25Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/MonotonicClock.java

+  @Override
+  void close();
+
+  void waitUntilTimeMillisAdvanced();


Could be good to add javadocs here (and above), it's not immediately clear what this method is supposed to do (spin-wait until the clock ticks?).

Also neither this method nor sleepMillis throw InterruptedException, which is surprising since they are clearly blocking. It could be good to add an @implSpec note about how the interrupt flag is expected to be handled.

adutra · 2025-07-18T09:35:34Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/IdGeneratorSpec.java

+  Map<String, String> params();
+
+  @PolarisImmutable
+  interface BuildableIdGeneratorSpec extends IdGeneratorSpec {


This class looks odd? Afaict we could use ImmutableIdGeneratorSpec.builder() instead. The only difference seems to be that there is a default type here, whereas in IdGeneratorSpec there isn't, but it's certainly possible to overcome this limitation somehow.

adutra · 2025-07-18T09:37:30Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/SnowflakeIdGenerator.java

+
+  long timestampFromId(long id);
+
+  long timestampUtcFromId(long id);


"Timestamp UTC" does not make sense to me 🤔
From the implementation class, it seems you meant "timestamp since Unix Epoch" instead.

adutra · 2025-07-18T09:37:51Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/SnowflakeIdGenerator.java

+
+  long constructId(long timestamp, long sequence, long node);
+
+  long timestampFromId(long id);


I suppose this is understood as "since the Snowflake Epoch (2025-03-01)". Could be good to add javadocs to clarify.

adutra · 2025-07-18T09:38:43Z

persistence/nosql/idgen/api/src/main/java/org/apache/polaris/ids/api/SnowflakeIdGenerator.java

+
+public interface SnowflakeIdGenerator extends IdGenerator {
+  /** Offset of the snowflake ID generator since the 1970-01-01T00:00:00Z epoch instant. */
+  Instant EPOCH_OFFSET =


Also, the expression is unnecessarily complex, the below one is 100% equivalent and imho easier to grasp:

Instant.parse("2025-03-01T00:00:00Z")

adutra · 2025-07-18T09:41:17Z

.../nosql/idgen/impl/src/main/java/org/apache/polaris/ids/impl/SnowflakeIdGeneratorFactory.java

+
+          @Override
+          public long currentTimeMillis() {
+            return SnowflakeIdGenerator.EPOCH_OFFSET_MILLIS;


This looks suspicious as it is returning a fixed timestamp. Did you mean clockMillis.getAsLong()?

Suggested change

return SnowflakeIdGenerator.EPOCH_OFFSET_MILLIS;

return clockMillis.getAsLong();

So, I think that there are some special-casing that is happening and is leaking into our interfaces and implementation. See here as well: https://github.com/apache/polaris/pull/2131/files#r2214779625

We need the Snowflake ID Generation for Commit IDs and Entity Object IDs.

We need a Node ID. Node IDs cannot be Snowflake IDs because Snowflake IDs require a NodeID.

So, I actually think that we need a NodeIdGenerator concept for Node IDs and the concepts presented here for the other IDs. Right now, we are blending them into one interface and I think that is making things a bit confusing.

For example:

IdGeneratorFactory#buildSystemIdGenerator

IdGenerator#systemIdForNode

I think that we probably need to separate these concepts.

What do y'all think, @dimas-b & @snazy?

I tend to think node ID generation should be internal to the Snowflake ID code. A sharable NodeIdGenerator generator might be an overkill, but that is a secondary concern.

My initial comment stems from the fact that the clockMillis parameter is part of the method signature (thus part of the API) but is not used, so I don't know why it is there.

adutra · 2025-07-18T09:44:45Z

...nce/nosql/idgen/impl/src/main/java/org/apache/polaris/ids/impl/SnowflakeIdGeneratorImpl.java

+  }
+
+  @Override
+  public long constructId(long timestamp, long sequence, long nodeId) {


This method effectively bypasses the Id generator source. Maybe it shouldn't be public? Having two methods in the same API that generate IDs using different mechanisms seems error-prone. OTOH this method seems only called from timeUuidToId, in this same class, so it seems it could be private.

adutra · 2025-07-18T09:45:33Z

...nce/nosql/idgen/impl/src/main/java/org/apache/polaris/ids/impl/SnowflakeIdGeneratorImpl.java

+  public long generateId() {
+    var nodeId = idGeneratorSource.nodeId();
+    checkState(nodeId >= 0, "Cannot generate a new ID, shutting down?");
+    var nodeIdPattern = ((long) nodeId) << sequenceBits;


Nit: move this variable declaration to line 228. It's not needed before.

…ian-software/polaris into persistence-no-sql-0

Adding the IDGen

1104fec

github-project-automation bot added this to Basic Kanban Board Jul 18, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 18, 2025

adam-christian-software marked this pull request as draft July 18, 2025 01:48

dimas-b reviewed Jul 18, 2025

View reviewed changes

adutra reviewed Jul 18, 2025

View reviewed changes

snazy and others added 3 commits July 18, 2025 10:25

Adding the IDGen

44a3791

Merge branch 'persistence-no-sql-0' of https://github.com/adam-christ…

c32b81a

…ian-software/polaris into persistence-no-sql-0

Address Minor PR Comments

5110375

adam-christian-software force-pushed the persistence-no-sql-0 branch from f5c9552 to 5110375 Compare July 18, 2025 18:43

adam-christian-software added 3 commits July 18, 2025 11:49

Update branch

5f4b8bf

Update build break

c350b84

Update ID_EPOCH

d6fe01f

dimas-b requested a review from dennishuo July 21, 2025 20:32


		long timestampFromId(long id);

		long timestampUtcFromId(long id);


		long constructId(long timestamp, long sequence, long node);

		long timestampFromId(long id);

	return SnowflakeIdGenerator.EPOCH_OFFSET_MILLIS;
	return clockMillis.getAsLong();

WIP feat(idgen): Start Implementation of NoSQL with the ID Generation Framework #2131

Are you sure you want to change the base?

WIP feat(idgen): Start Implementation of NoSQL with the ID Generation Framework #2131

Uh oh!

Conversation

adam-christian-software commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

Uh oh!

Uh oh!

Uh oh!

dimas-b Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adam-christian-software commented Jul 18, 2025 •

edited

Loading

dimas-b Jul 18, 2025 •

edited

Loading