fix: complete Collation comparison logic for total ordering#28
fix: complete Collation comparison logic for total ordering#28
Conversation
Fix __eq__ to compare languages for ordinal collations and include absorbs_damage in equality. Fix __gt__ to use deterministic tuple comparison instead of returning False for all non-trivial cases. Add comprehensive test suite with 33 tests covering equality, ordering, total ordering properties, and derived operators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d5b7c8058
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
data_diff/abcs/database_types.py
Outdated
| if self.ordinal != other.ordinal: | ||
| return False |
There was a problem hiding this comment.
__eq__ now marks ordinal=None and ordinal=False as different, but __gt__ still cannot order that pair when the other fields match, so both a > b and b > a are False; with the current __lt__ definition this makes both a < b and b < a true (e.g., Collation(ordinal=None) vs Collation(ordinal=False)), which breaks antisymmetry and makes sort/max behavior order-dependent when a dialect leaves ordinal unset.
Useful? React with 👍 / 👎.
…cion - Replace unsafe_hash=True with custom __hash__ via _comparison_key() to fix __eq__/__hash__ contract violation (equal objects now always have equal hashes) - Remove country/language None-tolerance from __eq__ to fix equality transitivity violation (A==B, B==C but A!=C) - Use (0,)/(1,val) sentinel wrapping in _ordering_key() so None sorts distinctly from "" and False, fixing __eq__/__gt__ inconsistency - Distinguish ordinal=None from ordinal=False in both equality and ordering - Fix typos in docstring and field comments - Expand test suite from 33 to 46 tests: hash consistency, equality transitivity, None-vs-False edge cases, all sensitivity tiebreakers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix class docstring: Snowflake is "lesser" (preferred target), not "greater" — matches actual absorbs_damage logic - Clarify __gt__ fallthrough comment: absorbs_damage and ordinal are already resolved by that point - Document _ordering_key divergence from _comparison_key for ordinals Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
__eq__inCollationto compare languages for ordinal collations (e.g.,Albanian_BIN != Latin_BINin MS SQL) and includeabsorbs_damagein equality checks for consistency with ordering__gt__to use deterministic tuple comparison on(language, country, case_sensitive, accent_sensitive, lower_first)instead of returningFalsefor all non-trivial cases, establishing total orderingCloses #8
Test plan
uv run pytest tests/test_collation.py -v— all 33 tests passmake test-unit— no regressions in existing unit testsuv run ruff check .— no lint issuesuv run ruff format --check .— formatting clean🤖 Generated with Claude Code