Fix deterministic fingerprints for set-valued arguments#1345
Open
mattfaltyn wants to merge 4 commits into
Open
Fix deterministic fingerprints for set-valued arguments#1345mattfaltyn wants to merge 4 commits into
mattfaltyn wants to merge 4 commits into
Conversation
|
|
mattfaltyn
marked this pull request as ready for review
July 19, 2026 07:11
mattfaltyn
requested review from
pratikk-databricks
and removed request for
a team
July 19, 2026 07:11
ghanse
approved these changes
Jul 20, 2026
ghanse
left a comment
ghanse
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the contribution! Overall this LGTM
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes non-deterministic rule fingerprints caused by set/frozenset iteration order (and thus PYTHONHASHSEED), by canonicalizing set-valued arguments during normalization so equivalent rules always hash the same.
Changes:
- Canonicalize
set/frozensetnormalization by sorting normalized elements using their JSON representations. - Preserve
list/tupleordering (no cross-type comparisons) while still recursively normalizing nested values. - Add a regression test that forces opposite iteration orders for a mixed/nested set and asserts equal fingerprints.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/databricks/labs/dqx/utils.py |
Updates normalize_bound_args to sort normalized set/frozenset values deterministically and updates the docstring accordingly. |
tests/unit/test_rule.py |
Adds a focused unit test ensuring compute_rule_fingerprint is order-independent for set-valued arguments with nested/mixed values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ghanse
added
the
Approved to Merge
Add equivalence-to-plain-set and negative-case assertions to guard against the custom subclass masking behavior and the sort collapsing distinct inputs. Co-authored-by: Isaac
mwojtyczka
approved these changes
Jul 22, 2026
mwojtyczka
left a comment
mwojtyczka
left a comment
Contributor
There was a problem hiding this comment.
LGTM - added a few more assertions to stregthen the tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Linked issues
Resolves #1333
Tests
PYTHONHASHSEEDvalues produced one fingerprint.make fmtpassed Black, Ruff, mypy, and Pylint (10.00/10).make test: 1,447 passed.Documentation and Demos