You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "vignesh-manel (via GitHub)" <gi...@apache.org> on 2026/04/08 15:56:01 UTC
[PR] feat(labels): Add labels for polaris entities [polaris]
vignesh-manel opened a new pull request, #4146:
URL: https://github.com/apache/polaris/pull/4146
Add key-value label support to Catalog entities for organizing and filtering catalogs via the management REST API and CLI.
- Catalogs can now have optional key-value labels (e.g. env=prod, team=platform) set at creation or updated later
- Labels are returned in catalog responses and omitted entirely when not set
- UpdateCatalogRequest supports labels (replace) and clearLabels (remove all); the two are mutually exclusive
- When labels is omitted on update, existing labels are preserved (non-destructive by default)
- New --label key=value flag on catalogs create and catalogs update in the CLI
- New --label-filter key=value flag on catalogs list to filter catalogs by label
- New --clear-labels flag on catalogs update to explicitly remove all labels
- Validation enforces: key โค 128 chars, value โค 256 chars, max 20 labels per catalog
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [x] ๐งพ Updated `CHANGELOG.md` (if needed)
- [x] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3068057656
##########
spec/polaris-management-service.yml:
##########
@@ -41,6 +41,17 @@ paths:
get:
operationId: listCatalogs
description: List all catalogs in this polaris service
+ parameters:
+ - name: labelFilter
Review Comment:
thanks for this suggestion, this is better way, updated the code accordingly
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4146:
URL: https://github.com/apache/polaris/pull/4146#issuecomment-4446849705
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3066065796
##########
spec/polaris-management-service.yml:
##########
@@ -878,6 +889,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ labels:
+ type: object
+ x-extra-annotation: "@com.fasterxml.jackson.annotation.JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY)"
Review Comment:
I believe it is preferable to keep the OpenAPI definition language-neutral.
I believe `null` values are already not included. Could we represent empty labels as `null` here?
Alternatively, it might be a good idea to make a separate change to avoid serializing empty values across all endpoints (cf. #1955, https://lists.apache.org/thread/nfboq6yo01x3lw4t3d5gp78f6lgs5ddz).
WDYT?
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
Review Comment:
@vignesh-manel : If I may suggest moving CLI changes to a separate PR, I think it would simplify review (just different audiences usually review core and CLI changes).
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3068053912
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -2484,6 +2502,272 @@ public void testCreateAndUpdatePrincipalWithReservedProperties() {
managementApi.deletePrincipal(principal);
}
+ @Test
+ public void testCatalogLabelsCreateAndGet() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://my-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-create-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://my-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "prod").containsEntry("team", "platform");
+ }
+
+ @Test
+ public void testCatalogLabelsPreservedOnUpdateWhenOmitted() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://preserve-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-preserve-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://preserve-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "staging"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "staging");
+
+ // Update with labels omitted (null) โ existing labels must be preserved
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
Review Comment:
added helper method `updateCatalogLabels`
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -2484,6 +2502,272 @@ public void testCreateAndUpdatePrincipalWithReservedProperties() {
managementApi.deletePrincipal(principal);
}
+ @Test
+ public void testCatalogLabelsCreateAndGet() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://my-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-create-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://my-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "prod").containsEntry("team", "platform");
+ }
+
+ @Test
+ public void testCatalogLabelsPreservedOnUpdateWhenOmitted() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://preserve-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-preserve-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://preserve-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "staging"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "staging");
+
+ // Update with labels omitted (null) โ existing labels must be preserved
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(),
+ fetched.getProperties().toMap(),
+ null,
+ null,
+ null)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).containsEntry("env", "staging");
+ }
+ }
+
+ @Test
+ public void testCatalogLabelsReplacedOnUpdate() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://replace-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-replace-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://replace-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "dev", "old-key", "old-val"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+
+ // Update with explicit new labels โ fully replaces old labels
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(), null, null, Map.of("env", "prod"), null)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).containsEntry("env", "prod").doesNotContainKey("old-key");
+ }
+ }
+
+ @Test
+ public void testCatalogLabelsClearedWithClearLabelsField() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://clear-labels-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-clear-catalog");
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://clear-labels-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build());
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsKey("env");
+
+ // Clear all labels using clearLabels=true
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(), null, null, null, true)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).isEmpty();
+ }
+ }
+
+ @Test
+ public void testListCatalogsWithLabelFilter() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://filter-bucket/"))
+ .build();
+
+ String prodCatalogName = client.newEntityName("filter-prod-catalog");
+ String devCatalogName = client.newEntityName("filter-dev-catalog");
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(prodCatalogName)
+ .setProperties(new CatalogProperties("s3://filter-bucket/prod"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod"))
+ .build());
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(devCatalogName)
+ .setProperties(new CatalogProperties("s3://filter-bucket/dev"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "dev"))
+ .build());
+
+ // Filter by env=prod โ only prodCatalog should match
+ try (Response response =
+ managementApi.request("v1/catalogs", Map.of(), Map.of("labelFilter", "env=prod")).get()) {
Review Comment:
added helper method `listCatalogsByLabel`
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3074782279
##########
spec/polaris-management-service.yml:
##########
@@ -878,6 +889,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ labels:
+ type: object
+ x-extra-annotation: "@com.fasterxml.jackson.annotation.JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY)"
Review Comment:
Empty maps in REST responses are probably acceptable.
I wonder if we could tweak OpenAPI code generation to exclude empty values too. For example, if we add a more general purpose flag to this property in the YAML file and later use it to produce `JsonInclude` annotations on the generated code... WDYT?
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3066239514
##########
spec/polaris-management-service.yml:
##########
@@ -41,6 +41,17 @@ paths:
get:
operationId: listCatalogs
description: List all catalogs in this polaris service
+ parameters:
+ - name: labelFilter
Review Comment:
suggestion: rename to `label` and use `:` as the separator. A sample query could be `curl -X GET http://.../catalogs?label=env:prod&label=team:eng`... It reads nicer from my POV.
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #4146:
URL: https://github.com/apache/polaris/pull/4146#issuecomment-4213357881
Hi @vignesh-manel !
Thanks a lot for this! I took a brief look over the PR and I do not see anything that's technically worrisome. Sure, there are a few things, but those can be sorted out w/o a lot of hassle.
As this is a new feature, I just wonder whether it's better to go on the dev-mailing-list and start a discussion about this new feature to gather the community's opinion and eventually consensus. WDYT?
Robert
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3067972959
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
Review Comment:
Thanks for the review.
Removed the CLI changes from this PR, will create a different PR for CLI, once this is merged.
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3068055336
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisBaseEntity.java:
##########
@@ -120,6 +125,33 @@ public Map<String, String> getInternalPropertiesAsMap() {
return PolarisObjectMapperUtil.deserializeProperties(getInternalProperties());
}
+ /**
+ * Returns the labels attached to this entity. Labels are stored in internal properties under
+ * {@link #LABELS_INTERNAL_KEY} and are never empty โ an absent key means no labels.
+ */
+ @JsonIgnore
+ public Map<String, String> getLabels() {
+ String raw = getInternalPropertiesAsMap().get(LABELS_INTERNAL_KEY);
+ if (raw == null || raw.isBlank()) {
+ return Collections.emptyMap();
+ }
+ return PolarisObjectMapperUtil.deserializeProperties(raw);
+ }
+
+ /**
+ * Returns {@code true} if this entity matches all entries in {@code labelFilter}. An empty filter
+ * matches every entity.
+ */
+ @JsonIgnore
+ public boolean matchesLabelFilter(@Nonnull Map<String, String> labelFilter) {
Review Comment:
renamed to `containsAllLabels`
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4146: feat(labels): Add labels for polaris entities
URL: https://github.com/apache/polaris/pull/4146
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3066213093
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisBaseEntity.java:
##########
@@ -120,6 +125,33 @@ public Map<String, String> getInternalPropertiesAsMap() {
return PolarisObjectMapperUtil.deserializeProperties(getInternalProperties());
}
+ /**
+ * Returns the labels attached to this entity. Labels are stored in internal properties under
+ * {@link #LABELS_INTERNAL_KEY} and are never empty โ an absent key means no labels.
+ */
+ @JsonIgnore
+ public Map<String, String> getLabels() {
+ String raw = getInternalPropertiesAsMap().get(LABELS_INTERNAL_KEY);
Review Comment:
WDYT about modelling each label as a separate internal property? e.g. `polaris.entity.label.env=dev`?
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -2484,6 +2502,272 @@ public void testCreateAndUpdatePrincipalWithReservedProperties() {
managementApi.deletePrincipal(principal);
}
+ @Test
+ public void testCatalogLabelsCreateAndGet() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://my-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-create-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://my-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "prod").containsEntry("team", "platform");
+ }
+
+ @Test
+ public void testCatalogLabelsPreservedOnUpdateWhenOmitted() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://preserve-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-preserve-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://preserve-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "staging"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "staging");
+
+ // Update with labels omitted (null) โ existing labels must be preserved
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(),
+ fetched.getProperties().toMap(),
+ null,
+ null,
+ null)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).containsEntry("env", "staging");
+ }
+ }
+
+ @Test
+ public void testCatalogLabelsReplacedOnUpdate() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://replace-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-replace-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://replace-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "dev", "old-key", "old-val"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+
+ // Update with explicit new labels โ fully replaces old labels
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(), null, null, Map.of("env", "prod"), null)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).containsEntry("env", "prod").doesNotContainKey("old-key");
+ }
+ }
+
+ @Test
+ public void testCatalogLabelsClearedWithClearLabelsField() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://clear-labels-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-clear-catalog");
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://clear-labels-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build());
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsKey("env");
+
+ // Clear all labels using clearLabels=true
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
+ .put(
+ Entity.json(
+ new UpdateCatalogRequest(
+ fetched.getEntityVersion(), null, null, null, true)))) {
+ assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+ Catalog updated = response.readEntity(Catalog.class);
+ assertThat(updated.getLabels()).isEmpty();
+ }
+ }
+
+ @Test
+ public void testListCatalogsWithLabelFilter() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://filter-bucket/"))
+ .build();
+
+ String prodCatalogName = client.newEntityName("filter-prod-catalog");
+ String devCatalogName = client.newEntityName("filter-dev-catalog");
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(prodCatalogName)
+ .setProperties(new CatalogProperties("s3://filter-bucket/prod"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod"))
+ .build());
+
+ managementApi.createCatalog(
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(devCatalogName)
+ .setProperties(new CatalogProperties("s3://filter-bucket/dev"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "dev"))
+ .build());
+
+ // Filter by env=prod โ only prodCatalog should match
+ try (Response response =
+ managementApi.request("v1/catalogs", Map.of(), Map.of("labelFilter", "env=prod")).get()) {
Review Comment:
nit: new helper method in `ManagementApi`?
##########
spec/polaris-management-service.yml:
##########
@@ -41,6 +41,17 @@ paths:
get:
operationId: listCatalogs
description: List all catalogs in this polaris service
+ parameters:
+ - name: labelFilter
Review Comment:
suggestion: rename to `label` and use `:` as the separator. A sample query could be `curl -X GET http://.../catalogs?label=env:prod&label=team:eng`... I reads nicer from my POV.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -254,13 +257,41 @@ public Response updateCatalog(
/** From PolarisCatalogsApiService */
@Override
- public Response listCatalogs(RealmContext realmContext, SecurityContext securityContext) {
- List<Catalog> catalogList = adminService.listCatalogs();
+ public Response listCatalogs(
+ List<String> labelFilter, RealmContext realmContext, SecurityContext securityContext) {
+ Map<String, String> parsedFilter = parseLabelFilter(labelFilter);
+ List<Catalog> catalogList = adminService.listCatalogs(parsedFilter);
Catalogs catalogs = new Catalogs(catalogList);
LOGGER.debug("listCatalogs returning: {}", catalogs);
return Response.ok(catalogs).build();
}
+ /**
+ * Parses a list of {@code "key=value"} label-filter strings into a map. Throws {@link
+ * BadRequestException} for malformed entries.
+ */
+ private static Map<String, String> parseLabelFilter(List<String> labelFilter) {
+ if (labelFilter == null || labelFilter.isEmpty()) {
+ return Collections.emptyMap();
+ }
+ Map<String, String> result = new HashMap<>(labelFilter.size());
+ for (String entry : labelFilter) {
+ int idx = entry.indexOf('=');
Review Comment:
We never actually process a label's key/value parts individually, right? Do we need to parse it at all?
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisBaseEntity.java:
##########
@@ -120,6 +125,33 @@ public Map<String, String> getInternalPropertiesAsMap() {
return PolarisObjectMapperUtil.deserializeProperties(getInternalProperties());
}
+ /**
+ * Returns the labels attached to this entity. Labels are stored in internal properties under
+ * {@link #LABELS_INTERNAL_KEY} and are never empty โ an absent key means no labels.
+ */
+ @JsonIgnore
+ public Map<String, String> getLabels() {
+ String raw = getInternalPropertiesAsMap().get(LABELS_INTERNAL_KEY);
+ if (raw == null || raw.isBlank()) {
+ return Collections.emptyMap();
+ }
+ return PolarisObjectMapperUtil.deserializeProperties(raw);
+ }
+
+ /**
+ * Returns {@code true} if this entity matches all entries in {@code labelFilter}. An empty filter
+ * matches every entity.
+ */
+ @JsonIgnore
+ public boolean matchesLabelFilter(@Nonnull Map<String, String> labelFilter) {
Review Comment:
nit: how about `containsAllLabels`? I think it would represent actual behaviour better.
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisBaseEntity.java:
##########
@@ -31,6 +33,9 @@ public class PolarisBaseEntity extends PolarisEntityCore {
public static final String EMPTY_MAP_STRING = "{}";
+ /** Internal-property key under which entity labels are stored as a serialized JSON map. */
+ public static final String LABELS_INTERNAL_KEY = "polaris.entity.labels";
Review Comment:
`protected` seems sufficient... Let's not expand the public API surface in "core" unless really necessary.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -2484,6 +2502,272 @@ public void testCreateAndUpdatePrincipalWithReservedProperties() {
managementApi.deletePrincipal(principal);
}
+ @Test
+ public void testCatalogLabelsCreateAndGet() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://my-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-create-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://my-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "prod", "team", "platform"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "prod").containsEntry("team", "platform");
+ }
+
+ @Test
+ public void testCatalogLabelsPreservedOnUpdateWhenOmitted() {
+ AwsStorageConfigInfo storageConfig =
+ AwsStorageConfigInfo.builder()
+ .setRoleArn("arn:aws:iam::123456789012:role/my-role")
+ .setExternalId("externalId")
+ .setUserArn("userArn")
+ .setStorageType(StorageConfigInfo.StorageTypeEnum.S3)
+ .setAllowedLocations(List.of("s3://preserve-bucket/"))
+ .build();
+ String catalogName = client.newEntityName("labels-preserve-catalog");
+
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setProperties(new CatalogProperties("s3://preserve-bucket/data"))
+ .setStorageConfigInfo(storageConfig)
+ .setLabels(Map.of("env", "staging"))
+ .build();
+
+ managementApi.createCatalog(catalog);
+
+ Catalog fetched = managementApi.getCatalog(catalogName);
+ assertThat(fetched.getLabels()).containsEntry("env", "staging");
+
+ // Update with labels omitted (null) โ existing labels must be preserved
+ try (Response response =
+ managementApi
+ .request("v1/catalogs/{name}", Map.of("name", catalogName))
Review Comment:
nit: maybe add a helper method to `ManagementApi`?
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on PR #4146:
URL: https://github.com/apache/polaris/pull/4146#issuecomment-4214530114
> Hi @vignesh-manel !
>
> Thanks a lot for this! I took a brief look over the PR and I do not see anything that's technically worrisome. Sure, there are a few things, but those can be sorted out w/o a lot of hassle.
>
> As this is a new feature, I just wonder whether it's better to go on the dev-mailing-list and start a discussion about this new feature to gather the community's opinion and eventually consensus. WDYT?
>
> Robert
@snazy Thanks for taking a look, I have posted it in dev mail thread: https://lists.apache.org/thread/2ny3gx01dx85b3bzo58zkbddryrjhfr3
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3068054629
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisBaseEntity.java:
##########
@@ -31,6 +33,9 @@ public class PolarisBaseEntity extends PolarisEntityCore {
public static final String EMPTY_MAP_STRING = "{}";
+ /** Internal-property key under which entity labels are stored as a serialized JSON map. */
+ public static final String LABELS_INTERNAL_KEY = "polaris.entity.labels";
Review Comment:
updated to `protected`
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "vignesh-manel (via GitHub)" <gi...@apache.org>.
vignesh-manel commented on code in PR #4146:
URL: https://github.com/apache/polaris/pull/4146#discussion_r3068047415
##########
spec/polaris-management-service.yml:
##########
@@ -878,6 +889,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ labels:
+ type: object
+ x-extra-annotation: "@com.fasterxml.jackson.annotation.JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY)"
Review Comment:
removed this, now the response will have labels as empty map {} , similar to other collection type attributes in the APIs
--
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] feat(labels): Add labels for polaris entities [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4146:
URL: https://github.com/apache/polaris/pull/4146#issuecomment-4231001944
Hi @vignesh-manel, thanks for the PR. Left a few questions in the dev mailing list. I think we need to think through the solution before jumping to an implementation.
--
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