NoSQL: Metastore implementation #3237

snazy · 2025-12-08T14:09:02Z

No description provided.

dennishuo

Still want to take a closer look at the rename/move logic, but the basic updateIfNotChanged flow looks good to me.

Left some initial comments. In general, it would be nice to expand javadoc comments whenever there are implicit format/parsing conventions defined in code, such as in the GrantTriplet (or class javadoc can link to wherever it's explained elsewhere if I just didn't see the file where it's documented).

dennishuo · 2025-12-11T06:03:28Z

...etastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/ContentIdentifier.java

+    return identifier(namespace.levels());
+  }
+
+  static ContentIdentifier identifierFromLocationString(String locationString) {


All the other factory methods are mostly self-explanatory, but this one could use some javadocs explaining what locationString is, expected format, etc.

dennishuo · 2025-12-11T06:08:46Z

...etastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/ContentIdentifier.java

+    var off = -1;
+    for (var i = 0; i < len; i++) {
+      var c = locationString.charAt(i);
+      checkArgument(c >= ' ', "Control characters are forbidden in locations");


Is this really intended to support all chars with ascii codepoint > ' ' or is there a more intuitive regex? In particular, though I don't know all ascii codes offhand, a cursory search seems to indicate for example that 127 is the "Delete control character" even though it comes after all the basic symbols and letters.

Maybe Character.isJavaIdentifierPart will help provide a better standard constrained set while also making it easier to reason about when reading the code.

This identifierFromLocationString is used (only) for location-overlap handling, assuming that the storage locations are already validated. Adding assumptions here about the actual format of object-store specific conventions or valid file-path chars feels a bit too risky?

dennishuo · 2025-12-11T06:12:56Z

...etastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/ContentIdentifier.java

+    for (var i = 0; i < len; i++) {
+      var c = locationString.charAt(i);
+      checkArgument(c >= ' ', "Control characters are forbidden in locations");
+      if (c == '/' || c == '\\') {


Is this for linux vs windows path separators? Can we just use java.nio.Path.of(...).iterator() instead?

Similar to the above comment: we'd assume that it's a file-system path (NIO Path delegates to file-system implementations), but object-store conventions do not match the the restrictions/validations.

That raises an interesting point - we'd ideally be able to incorporate storage-type-specific canonicalization logic.

In this code for example, presumably the \\ is specifically "windows local filesystem" use cases (which really should only be very limited test/dev scenarios where overlap-checks probably aren't relevant anyways; any "sensitive" production use cases would be really weird to run on a single-node Windows environment with engine and Polaris catalog in the same place, or else some NFS mount).

But for a nio Filesystem, it's actually correct to canonicalize with real filesystem syntax, such as collapsing "foo/./././././bar" into "foo/bar" or "foo/bar/.." becoming "foo" when it comes to "overlap detection" because overlap detection is precisely supposed to prevent security issues by sharing directories. So it becomes important to avoid creative syntax circumventing our check.

And yet, as mentioned before, the "local filesystem" use cases where paths have fancy syntactic interpretations also happen to be the cases where overlap-check probably doesn't actually matter.

I guess it's fine to leave as-is for now, but custom parsing of paths is probably not something we want to keep as ad-hoc inline logic here in the long-term. It might be worth adding a comment or filing a github issue to call out that this logic isn't necessarily authoritative for any future developers who might read it as gospel.

I think this is probably the wrong place to introduce a canonization. This piece of the code just deals with what's been configured. The configured values should have already been canonicalized.

Overall, we should probably revisit the location-overlap-checks.

dennishuo · 2025-12-11T09:00:32Z

...e/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java

+                new CreateCatalogResult(catalog, catalogAdminRole));
+          }
+          // retry
+          return new ChangeResult.NoChange<>(new CreateCatalogResult(ENTITY_ALREADY_EXISTS, null));


I'm not sure this fallthrough should exist; if returning ENTITY_ALREADY_EXISTS it's interpreted by the upper stack to mean the same name is already occupied but that this isn't actually an idempotent retry attempt (e.g. it had a different id).

Looks like line 232 already captures that case.

If we reach this line does it mean we're accruing a bunch of updates into the updatable indexes that we'll just discard when it comes time to "commit"?

If we want consistent behavior with the other metastore impls we would basically need a way to return NoChange but with a success CreateCatalogResult maybe in an else branch on line 230 (else if (catalogObj.stableId() == catalog.getId())).

It's possible the idempotent-retry scenario that code is trying to catch isn't actually possible anymore though, not sure.

I think the checkState(!byId.contains(idKey on line 246 means that this line wouldn't actually represent stableId == id though, in which case reaching this line should maybe be an IllegalStateException indicating some kind of corrupted state? Unless I'm misunderstanding the state of the index.

Basically we'd reach here if the byName already exists, and catalogObj.stableId() != catalog.getId(), but somehow catalogObj == null so that line 230 doesn't already exit early.

Is it expected for the byName index to not be atomic with what's visible in persistence.fetch?

I think all these subtleties could use some code comments to clarify if this is working as intended.

Both (index and persistence state) are always consistent. This particular one is literally an impossible case, so I removed it.

This (and other) functions have to expect cases when call sites (management API handlers) pass already existing names and/or already existing entity IDs. For example, a call site can pass a non-existing catalog name with an already used entity ID. Would be easier if the IDs were generated exclusively by the implementations, not the call sites.

I think the checkState(!byId.contains(idKey on line 246 means that this line wouldn't actually represent stableId == id though, in which case reaching this line should maybe be an IllegalStateException indicating some kind of corrupted state?
Actually not a corrupted state, but the API call sites passing an already used catalog ID into the "createCatalog" call.

When I wrote this function, I thought about how a retry could actually happen, given that the existing implementations assume entity-ID-equality (the reason for the TODO at the beginning of the function).

dennishuo · 2025-12-11T09:08:16Z

...e/src/main/java/org/apache/polaris/persistence/nosql/metastore/mutation/MutationResults.java

+    anyChange = true;
+  }
+
+  void entityResultNoChange(PolarisBaseEntity entity) {


Could use some javadoc comments explaining the difference between entityResultNoChange and unchangedEntityResult

Ah, two functions for the same thing. Removed one.

dennishuo · 2025-12-11T09:11:46Z

...tastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/privs/GrantTriplet.java

+
+/**
+ * Represents the triplet of catalog-ID, entity-ID and type-code plus a reverse-or-key marker.
+ * String representations of this type are used as ACL names and "role" names.


Looks like there's an implicit encoding convention here regarding the r/ and d/ convention. Could use javadoc examples here or else a javadoc link to wherever else the convention is defined other than just in the code's parsing logic in here.

dennishuo · 2025-12-12T08:51:37Z

...e/src/main/java/org/apache/polaris/persistence/nosql/metastore/mutation/MutationAttempt.java

+        mutationResults.entityResult(ENTITY_CANNOT_BE_RENAMED);
+        return;
+      }
+      if (!byName.remove(originalNameKey)) {


Does byName represent a fixed overall version for the duration of this method or is it a read-through to the "head" version of persistence?

My understanding is that it's a fixed/snapshot that then collects updates to apply at the end, is that correct? As in, the "immutable object index" from your design doc?

If so, would that mean that since we already validated byName.get(originalNameKey) on line 326 that the remove here must succeed, or else we have a code bug? In that case this should probably be an assert/IllegalStateException rather than returning ENTITY_NOT_FOUND because some assumptions must have gone wrong.

Or if byName is indeed subject to concurrent removal from somewhere else please add some comments around here indicating that it's possible and maybe additional comments around any other accesses in this method to the index to help explain how we safely reconcile the changing state.

Yea - checkState is better here.

dennishuo · 2025-12-12T08:57:24Z

...e/src/main/java/org/apache/polaris/persistence/nosql/metastore/mutation/MutationAttempt.java

+      mutationResults.entityResult(ENTITY_NOT_FOUND);
+      return;
+    }
+    var originalRef = byName.get(originalNameKey);


What are the scenarios that can lead to this byName not having this originalNameKey that we just got from byId? Would it indicate byId and byName getting out of sync somehow?

There's no thinkable scenario actually.

Ideally I'd prefer to not have the byId index at all and just refer to entities by name.
But that requires a bigger change.

dennishuo · 2025-12-12T08:59:55Z

...e/src/main/java/org/apache/polaris/persistence/nosql/metastore/mutation/MutationAttempt.java

+      Optional<PolarisPrincipalSecrets> currentSecrets,
+      ObjBase originalObj) {
+    return mapToObj(entity, currentSecrets)
+        .updateTimestamp(originalObj.createTimestamp())


Is this intentional to use originalObj.createTimestamp as the updateTimestamp for the change-comparison object? If so, definitely needs some extensive comments explaining why.

If the intent is to canonicalize updateTimestamp to not be relevant for semantic comparison, it might be better to set it to 0L or something to indicate intentional/obvious missing values rather than having comparison objects floating around that have the createTimestamp written in.

Oops, should have been updateTimestamp.
Sure, this could be optimized, but the call sites would become more complicated.

dennishuo · 2025-12-12T09:06:28Z

...e/src/main/java/org/apache/polaris/persistence/nosql/metastore/mutation/MutationAttempt.java

+
+  private static final Logger LOGGER = LoggerFactory.getLogger(MutationAttempt.class);
+
+  public static ObjBase objForChangeComparison(


Please add javadoc comment for this one explaining intent and how it intends to canonicalize whichever fields for comparison.

Is this to detect cases where a real update actually happens to result in an entity whose full contents are unchanged, or is there a common concurrency situation that is expected to cause "no change" mutations (e.g. on some kind of retry?)

It's to detect whether a update is actually changing something.

dennishuo · 2025-12-12T09:20:11Z

...etastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/ContentIdentifier.java

+    for (var i = 0; i < len; i++) {
+      var c = locationString.charAt(i);
+      checkArgument(c >= ' ', "Control characters are forbidden in locations");
+      if (c == '/' || c == '\\') {


That raises an interesting point - we'd ideally be able to incorporate storage-type-specific canonicalization logic.

In this code for example, presumably the \\ is specifically "windows local filesystem" use cases (which really should only be very limited test/dev scenarios where overlap-checks probably aren't relevant anyways; any "sensitive" production use cases would be really weird to run on a single-node Windows environment with engine and Polaris catalog in the same place, or else some NFS mount).

But for a nio Filesystem, it's actually correct to canonicalize with real filesystem syntax, such as collapsing "foo/./././././bar" into "foo/bar" or "foo/bar/.." becoming "foo" when it comes to "overlap detection" because overlap detection is precisely supposed to prevent security issues by sharing directories. So it becomes important to avoid creative syntax circumventing our check.

And yet, as mentioned before, the "local filesystem" use cases where paths have fancy syntactic interpretations also happen to be the cases where overlap-check probably doesn't actually matter.

I guess it's fine to leave as-is for now, but custom parsing of paths is probably not something we want to keep as ad-hoc inline logic here in the long-term. It might be worth adding a comment or filing a github issue to call out that this logic isn't necessarily authoritative for any future developers who might read it as gospel.

github-project-automation bot added this to Basic Kanban Board Dec 8, 2025

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

dimas-b requested a review from dennishuo December 8, 2025 20:05

snazy force-pushed the nosql-ms-impl branch from 63cf149 to 8b1ef72 Compare December 9, 2025 13:38

dimas-b previously approved these changes Dec 9, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 9, 2025

dennishuo reviewed Dec 11, 2025

View reviewed changes

snazy dismissed dimas-b’s stale review via 051114a December 11, 2025 10:12

dennishuo reviewed Dec 12, 2025

View reviewed changes

snazy added 9 commits December 12, 2025 16:51

NoSQL: Metastore implementation

c0656d7

Remove a actually unnecessary parameter

3bfd728

adapt test

cad4692

ci javadoc

c629810

rm unnecessary fallthrough

2b6f81e

rm unnecessary duplication

489e309

gt javadoc

8e65b35

review

e73f87a

rebase-fix

4d07106

snazy force-pushed the nosql-ms-impl branch from 34d73b4 to 4d07106 Compare December 12, 2025 15:51


		private static final Logger LOGGER = LoggerFactory.getLogger(MutationAttempt.class);

		public static ObjBase objForChangeComparison(

NoSQL: Metastore implementation #3237

Are you sure you want to change the base?

NoSQL: Metastore implementation #3237

Uh oh!

Conversation

snazy commented Dec 8, 2025

Uh oh!

dennishuo left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants