Skip to content

fix(ingest): allow full URNs and global tags for tags_to_ignore_sampling#18099

Merged
kevinkarchacryl merged 5 commits into
masterfrom
fix-tags-for-sampling
Jul 7, 2026
Merged

fix(ingest): allow full URNs and global tags for tags_to_ignore_sampling#18099
kevinkarchacryl merged 5 commits into
masterfrom
fix-tags-for-sampling

Conversation

@kevinkarchacryl

Copy link
Copy Markdown
Contributor

There are two issues with tags_to_ignore_sampling

The first is that if your tag is urn:li:tag:my_tag then you have to put my_tag in the recipe. I changed this so using the URN prefix will work as well. The documentation is also updated to make this clear.

The larger fix is that tags_to_ignore_sampling only used editableSchemaMetadata which does not account for tags that were ingested via a source. Tags for those columns are stored in the schemaMetadata.

@github-actions github-actions Bot added the ingestion PR or Issue related to the ingestion of metadata label Jun 30, 2026
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 3.57143% with 27 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...n/src/datahub/ingestion/source/ge_data_profiler.py 0.00% 15 Missing ⚠️
.../source/sqlalchemy_profiler/sqlalchemy_profiler.py 7.69% 12 Missing ⚠️

📢 Thoughts on this report? Let us know!

@datahub-connector-tests

datahub-connector-tests Bot commented Jun 30, 2026

Copy link
Copy Markdown

Connector Tests Results

All connector tests passed for commit 84243f7

View full test logs →

To skip connector tests, add the skip-connector-tests label (org members only).

Autogenerated by the connector-tests CI pipeline.

@maggiehays maggiehays added the needs-review Label for PRs that need review from a maintainer. label Jun 30, 2026

@treff7es treff7es left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix — reading both schemaMetadata (source-ingested tags) and editableSchemaMetadata closes a real gap, and the set-based collection also removes the old double-count when a column carried two matching tags. A few suggestions inline; none of them block the correctness of the fix.

Comment thread metadata-ingestion/src/datahub/ingestion/source/ge_data_profiler.py Outdated
Comment thread metadata-ingestion/src/datahub/ingestion/source/ge_data_profiler.py Outdated
Comment thread metadata-ingestion/src/datahub/ingestion/source/ge_data_profiler.py
Comment thread metadata-ingestion/tests/unit/profiling/test_columns_to_ignore_sampling.py Outdated
@maggiehays maggiehays added pending-submitter-merge and removed needs-review Label for PRs that need review from a maintainer. labels Jul 1, 2026
@kevinkarchacryl

Copy link
Copy Markdown
Contributor Author

@treff7es I forgot that I needed to change the SQL alchemy profiler as well. After the latest changes, this worked in the actual instance with tags ingested from Snowflake.

@kevinkarchacryl kevinkarchacryl requested a review from treff7es July 2, 2026 14:17
@kevinkarchacryl kevinkarchacryl merged commit 8fa3dc0 into master Jul 7, 2026
73 of 74 checks passed
@kevinkarchacryl kevinkarchacryl deleted the fix-tags-for-sampling branch July 7, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingestion PR or Issue related to the ingestion of metadata pending-submitter-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants