You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2026/09/03 01:52:49 UTC
[PR] Core: Clarify catalog path requirements [polaris]
flyrain opened a new pull request, #5436:
URL: https://github.com/apache/polaris/pull/5436
## Summary
`PolarisMetaStoreManager` documented non-null empty source paths as valid for top-level entities, while the transactional resolver rejects every non-null empty path. That mismatch could lead callers to inputs that are guaranteed to fail validation.
This change clarifies that:
- `null` represents a top-level path
- a supplied path must be non-empty and begin with a catalog
The clarification is applied consistently to listing, rename/drop, and transactional resolver Javadocs. There is no runtime behavior change.
## Testing
- `./gradlew format compileAll`
- `./gradlew :polaris-core:check`
- `./gradlew check` completed 2,849 tests with one unrelated failure: `RestCatalogKeycloakFileIT` could not obtain the Keycloak admin token because the endpoint returned HTTP 403. This matches the known macOS and Docker Desktop integration-test issue described in #2501.
## Checklist
- [x] I have not included any security-sensitive information.
- [x] I have explained why this change is needed.
- [x] I have verified the documentation change with formatting, compilation, and the `polaris-core` checks.
- [x] No tests are added because this change only corrects Javadocs and does not alter behavior.
- [x] No changelog entry is needed because this does not change user-facing behavior.
- [x] No site documentation update is needed because the affected contract is documented in source Javadocs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core: Clarify catalog path requirements [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5436:
URL: https://github.com/apache/polaris/pull/5436#discussion_r3920328777
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -289,7 +289,8 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
* Rename an entity, potentially re-parenting it.
*
* @param callCtx call context
- * @param catalogPath path to that entity. Could be an empty list of the entity is a catalog.
+ * @param catalogPath path to the entity's parent. Must be null if the entity is top-level;
+ * otherwise, it must be non-empty and start with the catalog
* @param entityToRename entity to rename. This entity should have been resolved by the client
* @param newCatalogPath if not null, new catalog path
* @param renamedEntity the new renamed entity we need to persist. We will use this argument to
Review Comment:
The Javadoc now clarifies `catalogPath` must be null or non-empty, but `newCatalogPath` is still documented as merely "if not null" even though implementations (e.g., transactional) validate it via `PolarisEntityResolver`, which rejects non-null empty paths and requires the first element to be the catalog. Please align `newCatalogPath`’s Javadoc with the same contract to avoid callers passing values that will fail validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core: Clarify catalog path requirements [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5436:
URL: https://github.com/apache/polaris/pull/5436#discussion_r3927819901
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -119,8 +119,8 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
* the entity are required,use {@link #listFullEntities} instead.
*
* @param callCtx call context
- * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level,
- * like catalogs
+ * @param catalogPath path inside a catalog. If null, the entities to list are top-level, like
Review Comment:
Quick question on the list* Javadocs `AtomicOperationMetaStoreManager.listEntities` still treats an empty path like null (`catalogPath == null || catalogPath.isEmpty() ? 0L`), and that’s what JDBC uses.. The resolver rejecting empty makes sense.. Should the SPI docs keep “null or empty” for list, or is empty intended to be invalid for every implementation? LGTM either way...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core: Clarify catalog path requirements [polaris]
Posted by "nandorKollar (via GitHub) via issues" <is...@polaris.apache.org>.
nandorKollar merged PR #5436:
URL: https://github.com/apache/polaris/pull/5436
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Core: Clarify catalog path requirements [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5436:
URL: https://github.com/apache/polaris/pull/5436#discussion_r4057577584
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -119,8 +119,8 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
* the entity are required,use {@link #listFullEntities} instead.
*
* @param callCtx call context
- * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level,
- * like catalogs
+ * @param catalogPath path inside a catalog. If null, the entities to list are top-level, like
Review Comment:
I'd keep null as the portable caller contract. JDBC accepting an empty list doesn't make it safe across implementations, since the transactional resolver rejects it. That doesn't require JDBC to start rejecting empty lists in this docs-only PR. I'd clarify that distinction in the PR description, since “guaranteed to fail validation” currently reads as applying to every implementation.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -289,7 +289,8 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
* Rename an entity, potentially re-parenting it.
*
* @param callCtx call context
- * @param catalogPath path to that entity. Could be an empty list of the entity is a catalog.
+ * @param catalogPath path to the entity's parent. Must be null if the entity is top-level;
+ * otherwise, it must be non-empty and start with the catalog
* @param entityToRename entity to rename. This entity should have been resolved by the client
* @param newCatalogPath if not null, new catalog path
* @param renamedEntity the new renamed entity we need to persist. We will use this argument to
Review Comment:
Agreed on documenting the destination path too. One distinction to preserve: null newCatalogPath keeps the existing parent in both implementations, whereas null catalogPath denotes a top-level entity. I'd document newCatalogPath as null to retain the parent, otherwise a non-empty path to the new parent starting with the catalog. Non-blocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org