Fix schema equality for empty schemas and identifier fields - #3958
kevinjqliu wants to merge 3 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are covered by regression tests, with no unresolved blocking issues.
Pull request overview
Fixes Schema equality for empty schemas and unordered identifier field IDs.
Changes:
- Removes truthiness-based rejection of empty schemas.
- Compares identifier field IDs as sets.
- Adds regression tests.
File summaries
| File | Description |
|---|---|
tests/test_schema.py |
Adds equality regression tests. |
pyiceberg/schema.py |
Updates schema equality logic. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This seems fine to me since we always know that comparisons should be done against other Schemas.
| if len(self.columns) != len(other.columns): | ||
| return False | ||
|
|
||
| identifier_field_ids_is_equal = self.identifier_field_ids == other.identifier_field_ids |
There was a problem hiding this comment.
I haven't dug into this at all, but what if we just stored the identifier field IDs as a set?
There was a problem hiding this comment.
yea i think that might be a better long term design. Something like frozenset[int] would be representative
There was a problem hiding this comment.
Yeah, that would work great!
Rationale for this change
Fix two cases where equivalent schemas compare unequal:
Schemadefines__len__. Theif not othercheck makes evenSchema() == Schema()returnFalse. Remove it; the existingisinstancecheck still rejects non-schema values.[1, 2]and[2, 1]are equal, while different sets remain unequal.This matches Java’s
Schema.sameSchema(): compare fields in order and identifier IDs as sets, ignoring the schema ID.Are these changes tested?
Yes, 331 schema tests pass, including focused regressions for both cases.
Are there any user-facing changes?
Empty schemas and schemas with reordered identifier IDs now compare equal. Field comparison is unchanged.