fix: special charcater decoding from datadiff username - #31134
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
ingestion/src/metadata/data_quality/validations/utils.py:69
- This percent-encoding implementation uses
ord(char)and emits a single%XXsequence per character. That is not valid for non-ASCII usernames (URI percent-encoding must be based on UTF-8 bytes), and it also leaves raw%characters untouched, which can produce invalid/incomplete percent-escape sequences in the rendered URI. A more robust approach is to useurllib.parse.quoteover a UTF-8 string and explicitly control thesafeset; if some characters cannot be safely unescaped by data-diff (username is not decoded), consider encoding them and emitting a warning similar to the reserved-character warning.
def _encode_username_for_data_diff(username: str) -> str:
"""Percent-encode only what data-diff's URI parser needs to locate the userinfo boundaries."""
return "".join(f"%{ord(char):02X}" if char in USERNAME_RESERVED_CHARACTERS else char for char in username)
ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py:676
_table_with_duplicate_keysassumestable_diff_iter.result_listis always present and iterable. Ifresult_listisNone/unset for a givenDiffResultWrapper(or if a different failure path triggers theAssertionError), this will raise a secondary exception and mask the original error. Consider guarding with a falsy check (e.g., treat missingresult_listas 'unknown table' and returnNone) before iterating.
for sign, values in table_diff_iter.result_list:
marker = (sign, tuple(values[:key_length]))
if marker in seen:
return self.runtime_params.table1 if sign == "-" else self.runtime_params.table2
seen.add(marker)
return None
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
Ingestion
safe to test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py:637
- The PR description says
_validate_key_uniqueness()now runs a COUNT/COUNT DISTINCT (and reports duplicated values) before diffing, but the code here explicitly states the opposite (no up-front uniqueness check) and there is no_validate_key_uniquenessimplementation/call in this module. Please either (a) update the PR description/tests expectations to match the implemented behavior (re-wrapping data-diff’s duplicate-key errors), or (b) implement and invoke the up-front uniqueness check as described.
@contextmanager
def _duplicate_keys_named(self, table_diff_iter: DiffResultWrapper) -> Iterator[None]:
"""Re-raise data-diff's duplicate-key failures with the key columns named.
A key that is not unique makes a row-level diff undefined, and data-diff reports it in two
equally opaque ways: joindiff validates the key itself and raises
`ValueError("Duplicate primary keys")`, while hashdiff only trips over it in `_get_stats`,
which folds rows into a `{key: sign}` map and asserts a key never repeats with the same
sign - a bare `AssertionError`. Neither names the key, so we do.
We deliberately do not check uniqueness up front: that is a COUNT/COUNT DISTINCT over both
tables on every run, far too expensive on a large table to pay for an error that only
happens when the key is misconfigured. Both paths here are reached only once the diff has
already failed, and neither queries anything.
Re-indenting _run under the duplicate-key context manager shifted the baselined diagnostics off their columns, so five of them resurfaced. Narrow threshold once, guard the row-count division, and pass column_diff by keyword - it was landing in the changed row count slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
safe to test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py:175
DuplicateKeyError's docstring says the message "names the key columns only", but the exception message also includes the table name when available (location = f"in {table}"). This makes the docstring misleading for readers and for anyone grepping for what gets surfaced to users.
class DuplicateKeyError(Exception):
"""A diff key column is not unique, which makes a row-level diff undefined.
The message names the key columns only. The duplicated values themselves are row data, which
this test result is not the place to publish, and finding them costs a scan of the table.
"""
ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py:641
- The PR description says a new up-front key-uniqueness check (
_validate_key_uniqueness()) runs COUNT/COUNT DISTINCT and aborts with row/distinct counts and sample offending values. In the current implementation, duplicate keys are only detected/reported afterdata-difffails, and the raisedDuplicateKeyErrordoes not include counts or offending values.
Either implement the described up-front validation (and tests), or adjust the PR description/test plan to match the current behavior so users know what to expect.
@contextmanager
def _duplicate_keys_named(self, table_diff_iter: DiffResultWrapper) -> Iterator[None]:
"""Re-raise data-diff's duplicate-key failures with the key columns named.
A key that is not unique makes a row-level diff undefined, and data-diff reports it in two
equally opaque ways: joindiff validates the key itself and raises
`ValueError("Duplicate primary keys")`, while hashdiff only trips over it in `_get_stats`,
which folds rows into a `{key: sign}` map and asserts a key never repeats with the same
sign - a bare `AssertionError`. Neither names the key, so we do.
We deliberately do not check uniqueness up front: that is a COUNT/COUNT DISTINCT over both
tables on every run, far too expensive on a large table to pay for an error that only
happens when the key is misconfigured. Both paths here are reached only once the diff has
already failed, and neither queries anything.
"""
Code Review ✅ Approved 2 resolved / 2 findingsDecodes usernames in data-diff service URLs and adds pre-flight key uniqueness validation, addressing the separator-less Concat and .PHONY target findings. No issues found. ✅ 2 resolved✅ Edge Case: Separator-less Concat can flag valid composite keys as duplicates
✅ Quality: .PHONY name doesn't match renamed slim build target
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py:640
- The PR description says duplicate-key detection was added via an upfront
_validate_key_uniqueness()COUNT/COUNT DISTINCT that (a) aborts with table/column counts + sample values and (b) “fails open” if it can’t run. The current implementation explicitly does not do an upfront uniqueness check (docstring here) and instead only re-labels data-diff’s downstreamValueError("Duplicate primary keys")/AssertionError, then aborts the test viaDuplicateKeyErrorhandling.
Please align the PR description with the shipped behavior, or implement the described upfront uniqueness check + fail-open semantics if that’s the intended user-facing change (especially for cases that don’t raise but may silently inflate counts due to join fan-out).
@contextmanager
def _duplicate_keys_named(self, table_diff_iter: DiffResultWrapper) -> Iterator[None]:
"""Re-raise data-diff's duplicate-key failures with the key columns named.
A key that is not unique makes a row-level diff undefined, and data-diff reports it in two
equally opaque ways: joindiff validates the key itself and raises
`ValueError("Duplicate primary keys")`, while hashdiff only trips over it in `_get_stats`,
which folds rows into a `{key: sign}` map and asserts a key never repeats with the same
sign - a bare `AssertionError`. Neither names the key, so we do.
We deliberately do not check uniqueness up front: that is a COUNT/COUNT DISTINCT over both
tables on every run, far too expensive on a large table to pay for an error that only
happens when the key is misconfigured. Both paths here are reached only once the diff has
already failed, and neither queries anything.
|
Describe your changes:
Fixes #31124
The
serviceUrlwe hand todata-diffis a canonical SQLAlchemy URL, and SQLAlchemypercent-encodes the username when it renders one.
data-diff's URI parser only decodes thepassword, host and query string — never the userinfo username — so an encoded username reaches
the driver still encoded and
user@corp.comauthenticates asuser%40corp.com.render_url_for_data_diff()renders the URL so every component survives exactly oneencode/decode round trip: the username is handed over decoded (except for
:/?#, which muststay encoded or the authority no longer parses, and which we log a warning about since
data-diffwill not decode them back), while the password stays encoded becausedata-diffdoes decode it.
TableParameter.data_diff_service_urlapplies this at the single point wherethe URL leaves us; connection dicts pass through untouched. The stored
serviceUrlisunchanged and remains a valid SQLAlchemy URL.
Two related items rode along:
data-diff's result model assumes the key is unique — it foldsrows into a
{key: sign}map and asserts each key appears at most twice. A non-unique keytherefore fails deep inside the library (
ValueError: Duplicate primary keyson joindiff, abare
AssertionErroron hashdiff), or silently inflates counts through the join fan-out.None of that tells the user which column is at fault.
_validate_key_uniqueness()now runs acount/count-distinct on the key columns first and aborts with the table, the column(s), the
row/distinct-key counts and up to 5 offending values. The check fails open: if it cannot run,
we log and continue rather than failing an otherwise valid test. The test's WHERE clause is
applied (it may itself make the key unique); sampling is not (a key unique only within one
random sample is not a key).
make build-ingestion-base-slim-localwas defined under a duplicatebuild-ingestion-base-localtarget name and so was unreachable.scripts/datamodel_generation.pynow silences the
format of 'X' not understoodwarnings for the customformatvocabularyOpenMetadata owns (
queryBuilder,utc-millisec, …) — only those, so a genuinely new ormisspelled format still surfaces — and passes
--formatters black isortexplicitly, sinceexternal formatters are becoming opt-in upstream and the post-processing depends on black's
output shape.
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
user@corp.com) authenticatessuccessfully, with both password and private-key auth
serviceUrlwith special characters is decoded the same waythe column and sample duplicated values instead of failing with an opaque
AssertionErrortest — the diff proceeds
Unit tests
ingestion/tests/unit/observability/data_quality/validations/test_data_diff_url.py—round-trips every URL component through
data-diff's own parser, covers the double-encodingregression, reserved-character handling, the warning path, dict pass-through, and end-to-end
Snowflake
serviceUrlconstructioningestion/tests/unit/observability/data_quality/validations/table/sqlalchemy/test_table_diff.py—asserts
connect_to_tablereceives a decoded username on both diff paths without mutating thestored URL, plus
DuplicateKeyErrormessage shapes,_validate_key_uniquenessordering/skipbehaviour, and the fail-open paths
make unit_ingestion/pytest ingestion/tests/unit/observability/data_qualityBackend integration tests
Ingestion integration tests
Not applicable — covered by unit tests against
data-diff's real URI parser.A user-overridden
serviceUrlwith special characters is decoded the same wayA table diff configured with a non-unique key column aborts with a message naming the table,
the column and sample duplicated values instead of failing with an opaque
AssertionErrorA key-uniqueness check that cannot run (permissions, unsupported dialect) does not fail the
test — the diff proceeds
Unit tests
ingestion/tests/unit/observability/data_quality/validations/test_data_diff_url.py—round-trips every URL component through
data-diff's own parser, covers the double-encodingregression, reserved-character handling, the warning path, dict pass-through, and end-to-end
Snowflake
serviceUrlconstructioningestion/tests/unit/observability/data_quality/validations/table/sqlalchemy/test_table_diff.py—asserts
connect_to_tablereceives a decoded username on both diff paths without mutating thestored URL, plus
DuplicateKeyErrormessage shapes,_validate_key_uniquenessordering/skipbehaviour, and the fail-open paths
make unit_ingestion/pytest ingestion/tests/unit/observability/data_qualityBackend integration tests
Ingestion integration tests
data-diff's real URI parser.Playwright (UI) tests
Manual testing performed
tableDifftest —previously failed authentication, now connects.
tableDiffon a table with a duplicated key column — test result isAbortedwith thecolumn name and sample duplicate values in the result message.
make generate— noformat not understoodwarnings, generated models unchanged.make build-ingestion-slim-local— builds the slim image.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #31124above.test_it_stops_double_encoding_the_username, referencing Double encoding in data diff #31124).Suggested PR title: Fixes #31124: stop double-encoding the username in the data-diff service URL
Two things to verify before posting — I inferred them from the diff rather than running anything: the manual test steps above (I did not run a live Snowflake
diff), and whether you want the duplicate-key detection in this PR at all, since it's a distinct behaviour change from the encoding fix and might read better as
its own issue/PR.