Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
CI is blocked by the repository label gate. A maintainer needs to add the \safe to test\ label before the required checks can run past the label verification step. |
Code Review ✅ ApprovedRefactors table column tag hydration to ensure accurate property mapping. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Pull request overview
Fixes missing column-level tag hydration for table reads when only fields=columns is requested, while keeping table-level tags gated behind fields=tags for bulk reads (per #27519).
Changes:
- Hydrate column tags whenever
columnsare requested in single-table reads (setFields). - Update bulk hydration to fetch column tags when
columnsare requested, but only fetch table tags whentagsare requested. - Add regression tests covering single-table and bulk hydration paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java | Ensures column tag hydration runs when columns are requested; avoids bulk table-tag fetch unless tags is requested. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TableRepositoryColumnTagsTest.java | Adds regression coverage for column tag hydration in both single-entity and bulk paths. |
| @Test | ||
| void setFieldsInBulk_hydratesColumnTagsWhenOnlyColumnsAreRequested() { | ||
| Table table = tableWithColumn("service.database.schema.users", "email"); | ||
| TagLabel piiTag = | ||
| new TagLabel() | ||
| .withTagFQN("PII.Sensitive") | ||
| .withSource(TagSource.CLASSIFICATION) | ||
| .withLabelType(LabelType.MANUAL) | ||
| .withState(State.CONFIRMED); | ||
| String columnFqn = table.getColumns().getFirst().getFullyQualifiedName(); | ||
|
|
||
| when(tagUsageDAO.getTagsInternalBatch(anyList())) | ||
| .thenReturn(List.of(tagUsage(columnFqn, piiTag))); | ||
|
|
||
| repository.setFieldsInBulk( | ||
| new Fields(repository.getAllowedFields(), "columns"), List.of(table)); | ||
|
|
||
| assertNull(table.getTags()); | ||
| assertColumnTags(table, piiTag); | ||
| } |
There was a problem hiding this comment.
In setFieldsInBulk_hydratesColumnTagsWhenOnlyColumnsAreRequested, assertNull(table.getTags()) doesn’t prove bulk table-level tags were not fetched, since clearFieldsInternal will null out table.tags whenever fields doesn’t include tags (even if they were loaded). To fully regression-test the “keep top-level tags gated behind fields=tags” behavior, add a Mockito verification that the tag DAO is only queried for the column FQN(s) (and not also for the table FQN) when Fields is just columns.
Summary
fields=tagsin bulk table reads.Fixes #27519
Validation
git diff --cached --checkgh pr checks 27522 --repo open-metadata/OpenMetadataas the current green Java-service CI baselinemvn -pl openmetadata-service -Dtest=TableRepositoryColumnTagsTest testlocally becausemvnandjavaare not installed in this environment.