You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyingImer (via GitHub)" <gi...@apache.org> on 2026/09/08 21:07:52 UTC
[PR] Polaris Tag Management: tag assignment write path [polaris]
flyingImer opened a new pull request, #5469:
URL: https://github.com/apache/polaris/pull/5469
> [!TIP]
> Until #5391 merges, the files view includes commits from #5366 and #5391 too. For this slice's
> own diff, **review the head commit: https://github.com/flyingImer/polaris/commit/e32be42e11e04c2e1a314514d731cd863af2ac5e**.
**GH proposal tracking**: https://github.com/apache/polaris/issues/5442
## Why
#5391 makes Tag definitions usable, but nothing can be attached to them yet: an authorized
`detach-all=true` request on drop answers 501, and there is no way to put a tag on anything. This
PR makes tags attachable. A tag can be assigned to a catalog, a namespace, an Iceberg or generic
table as a whole object, or a top-level column of an Iceberg table, one selected value per
definition and target, and unassigned again. Dropping
a definition with `detach-all=true` now removes the definition and every assignment together, or
nothing at all, replacing the transitional 501.
The series: spec (#5366), definition CRUD (#5391), this PR (assignment writes), then reads
(direct/effective views, reverse lookup) next. Design doc:
https://docs.google.com/document/d/1rIJGzcsmGhfrBiRXPac51hr-jeJuuKQQBYjgBdOb9-k/edit. Dev-list
discussion: https://lists.apache.org/thread/n2p8rvgno67tv25b3f3kpwlj7bzt0421.
This PR will remain Draft until #5391 merges and the rebased SHA passes CI.
## What this PR does
* **Assign / unassign.** `PUT` and `POST` on `/tags/{tag}/mappings` write and remove one selected
value per tag definition and target. Assigning the same tag to the same target again is
create-or-replace: the new value replaces the old one. Column targets resolve to the containing
table's entity id plus its current Iceberg field id, read directly off the table's metadata, so
a column rename keeps the assignment and a dropped-and-recreated column does not inherit it.
* **Authorization.** Both operations check two securables: `TAG_ATTACH` or `TAG_DETACH` on the tag
definition, plus a target-side privilege (`{CATALOG,NAMESPACE,TABLE}_{ATTACH,DETACH}_TAG`) on
what is being tagged; a column assignment checks the containing table's privilege.
`CATALOG_MANAGE_CONTENT` covers every new privilege, so existing admin roles need no grant
changes.
* **Schema v7.** Assignments are stored in a new `tag_assignment_record` table. Writes on a
database below schema v7 fail with an error naming the v7 requirement; the upgrade is a one-time
manual SQL step, documented in the CHANGELOG and the metastore docs. Definitions are unaffected
either way; they live in the existing entities table.
* **The atomic combined delete.** `detach-all=true` on drop must remove the definition and every
assignment together, or leave both untouched; a backend that cannot guarantee that must reject
the operation rather than report success after partial work. The JDBC and transactional
in-memory backends both run the two deletes inside one transaction. That atomicity covers the two
deletes against each other, not against a concurrent assignment write: a write that read the tag
definition before the detach-all's delete commits can still insert its assignment afterward,
since neither side takes a lock the other would see. The result is an assignment row with no
definition behind it, the same shape of orphan the best-effort target-deletion cleanup already
produces, just on the definition side instead of the target side.
* **Target-deletion cleanup.** Dropping a catalog, namespace, or table removes its tag assignment
rows through the same best-effort step that already removes grant records and policy mappings on
drop, not a separate pass. A cleanup failure here never blocks the drop; any row left behind is
orphaned and hidden from reads, same as today's grant and policy-mapping orphans.
* **NoSQL stays unsupported.** The NoSQL backend has no tag type mapping and its generic drop path
deletes the entity before any type-specific cleanup runs, so it explicitly rejects tag
assignment operations and tag drops rather than risk a half-completed drop.
* **Value length.** Allowed values and assigned values are capped at 2000 UTF-8 bytes. The spec's
section 1.3 leaves size limits to the implementation; this PR sets this one because the selected
value is part of a database index key used for the later reverse lookup, and relational backends
cap an index entry at a fraction of a page. It's an implementation size limit, not a new v1
restriction.
One note on concurrency: assigning is create-or-replace, so the loser of a concurrent first-write
race on the same assignment lands the replacement value rather than an error; there is no
compare-and-swap on the assignment row itself, and the write never bumps the tag definition's
version. That replacement retry runs once, locally: a second consecutive conflict on the same
identity surfaces as a server error instead of retrying again, the same residual policy mapping
writes already carry today. The write also re-reads the definition inside that same transaction,
checks the selected value against its allowed values, and conditions its write on the definition's
version. That serializes assignment writes against a concurrent definition update on the same row:
an update that commits first makes the write re-validate against the new allowed-values list and
reject a value it removed, while an assignment that commits first is grandfathered in. Unassigning
an assignment that is already gone, or dropping a tag that is already gone, answers 404.
## Extension-point changes
Two mixin interfaces carry the assignment persistence contract, threaded into the existing
extension points the same way `PolicyMappingPersistence` was when policies landed:
`BasePersistence` gains `TagAssignmentPersistence` (six methods: `writeToTagAssignmentRecords`,
`deleteFromTagAssignmentRecords`, `deleteAllEntityTagAssignmentRecords`,
`lookupTagAssignmentRecord`, `loadAllTagAssignmentsOnTargetEntity`, `loadAllTargetsOnTag`, plus the
capability method below), and `TransactionalPersistence` gains
`TransactionalTagAssignmentPersistence` with the equivalent `InCurrentTxn` methods. Every one of
these is `default` and throws "this backend does not support tag assignments", so an out-of-tree
persistence backend keeps compiling and stays fail-closed until it opts in.
The capability method, `deleteTagAndAllAssignmentRecords`, lives on `TagAssignmentPersistence`
(so on `BasePersistence`) and is also `default`-throws. It backs the atomic combined delete: a
backend that cannot delete a tag and its assignments atomically must not override it, and every
caller treats `UnsupportedOperationException` from it as "this backend cannot do detach-all" and
rejects the operation instead of falling back to a non-atomic delete.
`PolarisMetaStoreManager` gains `PolarisTagAssignmentManager`, with two methods,
`assignTagToEntity` and `unassignTagFromEntity`. Unlike the persistence-level mixins, these are
plain abstract methods, not `default`, the same shape as `PolarisPolicyMappingManager`'s attach and
detach methods. An out-of-tree `PolarisMetaStoreManager` implementation will not compile against
this PR without adding both; "keeps compiling" holds for a persistence backend, not for a metastore
manager. All three in-tree implementations (`AtomicOperationMetaStoreManager`, `NoSqlMetaStoreManager`,
`TransactionWorkspaceMetaStoreManager`) add them; the NoSQL one returns a
`TAG_ASSIGNMENTS_NOT_SUPPORTED` result rather than throwing, matching its existing rejection style
for tags, while the transaction-workspace one throws, since assignment needs its own transaction.
A dev-list note describing this delta is posted on the design thread when this PR opens.
One acknowledgment: threading the tag mixin onto `BasePersistence` follows the precedent
`PolicyMappingPersistence` set, but that precedent's own reviewers flagged the inheritance
hierarchy as confusing and deferred a cleaner shape to a follow-up when policies landed. This PR
doesn't revisit that; the follow-up, if it happens, would cover both mixins together.
## Deferred
* **Grant surface.** Until a follow-up adds tag privileges to the management grant APIs, only
principals already holding `CATALOG_MANAGE_CONTENT` can assign or unassign; the new privileges
are not individually grantable yet.
* **Orphan-row reclamation.** Best-effort cleanup on target deletion can leave orphaned assignment
rows; the read slice (next PR) hides them, and reclaiming them is deferred to the same lifecycle
discussion grants and policy mappings already need. Tags stay behind the default-off
`ENABLE_TAG_STORE` gate in the meantime (see below).
* **Events.** Assign and unassign do ship events in this PR (`BEFORE_ASSIGN_TAG`,
`AFTER_ASSIGN_TAG`, `BEFORE_UNASSIGN_TAG`, `AFTER_UNASSIGN_TAG`), wired the same way #5391 wired
the definition-CRUD events.
* **Views, generic-table columns, nested fields, multi-value.** Same v1 exclusions #5366 states.
The write path answers 400 for a target or column shape it doesn't support (an Iceberg view, a
non-Iceberg column, more than one selected value, a null or empty path segment).
* Tags classify resources only; they do not grant or deny access in v1.
* Tags are disabled by default, behind `ENABLE_TAG_STORE`, unchanged from #5391.
* The schema gate is a write-side rejection only: a write on a database below schema v7 fails with
an error naming the v7 requirement. This PR does not change read behavior.
* **Cross-catalog check.** The schema carries no CHECK constraint pinning a target's catalog to
the tag definition's catalog; the manager enforces that invariant before every in-tree write
reaches persistence. Adding the constraint at the database layer would be hardening, not a
correctness gap today.
* **Exception layering.** The persistence write path throws Iceberg's `BadRequestException`, and a
Polaris `NoSuchTagException` that maps itself to 404, directly from persistence code. The status
each produces is correct; moving that decision out of persistence and into the REST layer is a
cleaner layering, left for follow-up.
## Testing
* `PolarisTagServiceIntegrationTest` covers assign/unassign on all four target kinds, including a
column resolved through a real metadata read, value validation and replacement semantics
including a narrowed allowed-values list, and the in-use-then-detach-all drop flow.
* `TagCatalogHandlerAuthzTest` covers each assign/unassign operation and the two-privilege
detach-all rule, including the case where a privilege that covers dropping does not cover
detaching.
* `JdbcBasePersistenceImplTest` adds conflict-injection tests: a failure injected between the two
deletes of the atomic combined delete, asserting both rows survive the rollback (a success-path
test alone can't tell atomic from happened-to-complete), plus tests for the write path's
conflict re-run behavior.
* `TagSchemaGateTest` pins the fail-closed write gate below schema v7.
* `AtomicMetastoreManagerWithJdbcBasePersistenceImplPostgresSchemaIT` runs the shared metastore
contract tests against PostgreSQL at every schema version from v1 to v7.
## AI-assisted contribution
AI assistance was used for implementation and consistency review. I reviewed the final behavior
and diff and take full responsibility for the contribution.
### Checklist
* [x] ๐ก๏ธ Don't disclose security issues! (contact [security@apache.org](mailto:security@apache.org))
* [x] ๐ Clearly explained why the changes are needed, with design and dev-list links
* [x] ๐งช Added/updated tests with good coverage, or manually tested
* [x] ๐ก Added comments for complex logic
* [x] ๐งพ Updated `CHANGELOG.md`
* [ ] ๐ Updated documentation in `site/content/in-dev/unreleased`
--
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] Polaris Tag Management: tag assignment write path [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5469:
URL: https://github.com/apache/polaris/pull/5469#issuecomment-6406411872
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] Polaris Tag Management: tag assignment write path [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer closed pull request #5469: Polaris Tag Management: tag assignment write path
URL: https://github.com/apache/polaris/pull/5469
--
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] Polaris Tag Management: tag assignment write path [polaris]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@polaris.apache.org>.
github-actions[bot] closed pull request #5469: Polaris Tag Management: tag assignment write path
URL: https://github.com/apache/polaris/pull/5469
--
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