Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ir): ensure that schema column order matters #9068

Closed
wants to merge 2 commits into from
Closed

fix(ir): ensure that schema column order matters #9068

wants to merge 2 commits into from

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Apr 27, 2024

Ensure that column order is taken into account for equality comparisons. Fixes #9063.

@cpcloud cpcloud added this to the 9.0 milestone Apr 27, 2024
@cpcloud cpcloud added the internals Issues or PRs related to ibis's internal APIs label Apr 27, 2024
@cpcloud cpcloud force-pushed the schemas-are-ordered branch from 4930e6b to 94c3db6 Compare April 27, 2024 14:36
@cpcloud cpcloud requested a review from kszucs April 27, 2024 14:37
@cpcloud cpcloud force-pushed the schemas-are-ordered branch 2 times, most recently from 1b4c0c0 to 6380b0f Compare April 27, 2024 14:42
kszucs
kszucs reviewed Apr 27, 2024
View reviewed changes
ibis/common/collections.py
def __eq__(self, other: Any) -> bool:
if not isinstance(other, collections.abc.Mapping):
return NotImplemented
return collections.OrderedDict(self) == collections.OrderedDict(other)
Copy link
Member

@kszucs kszucs Apr 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I assume the point of this change is to turn differently ordered projections unequal.

Since we use FrozenDict heavily across the codebase how about adding a FrozenOrderedDict(FrozenDict) for this purpose using all(a == b for a, b in zip_longest(self.items(), other.items())) for comparison?

Then FrozenOrderedDict could be used for the relevant relational operations instead of FrozenDict.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, let's do that after the 9.0 release!

@cpcloud cpcloud force-pushed the schemas-are-ordered branch from 6380b0f to 764fc94 Compare April 29, 2024 19:33
@cpcloud
Copy link
Member Author

cpcloud commented Apr 29, 2024

Clouds are passing:

…/ibis on  schemas-are-ordered is 📦 v8.0.0 via 🐍 v3.12.2 via ❄️   impure (ibis-3.12.2-env) took 9s
❯ pytest -m 'bigquery or snowflake' -n auto --dist loadgroup --snapshot-update
======================================================================================== test session starts =========================================================================================
platform linux -- Python 3.12.2, pytest-8.1.1, pluggy-1.5.0
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
Using --randomly-seed=3621327880
rootdir: /home/cloud/src/ibis
configfile: pyproject.toml
plugins: anyio-4.3.0, xdist-3.5.0, pytest_httpserver-1.0.10, cov-5.0.0, timeout-2.3.1, benchmark-4.0.0, randomly-3.15.0, hypothesis-6.100.1, repeat-0.9.3, snapshot-0.9.0, mock-3.14.0, clarity-1.0.1
16 workers [3586 items]
..x.xx.xx...x....xx...x...............x.x..................x.x........x...........x.........x........x...........................................x.......x...........................x.....sss [  5%]
sssssssssssssssssssx....................................................x...........x........................................x..............x...................x................x..........x. [ 10%]
.............x.x...xxx.....xx....x..x.x.........x....xx....x......x......x....x....x.xx............xx.........x.x....x.....x................x....x............x.....................x..x...... [ 15%]
.x.x................x..x.........xx...........x..x.xx................x..x.................x....x..........x..............s...s............................................s.......x......s.... [ 21%]
...x............x....s.....xs............xx..s.x..............xxxx.x.xxx.xx..x.......x..x.........x......x....x....x......x........x.......xxx.x.xxxxx.xx.x.x..x..x..x...xx.x.x.xx.xxx...xxx.x [ 26%]
.xx...x.x.xx....x....x..xx.xx.x.x...x.....xx...x.......xx....................x........x..x.....s...x.....x.......x.....x..........x.............x..xx.xx..xxxxxxxxxxx.xxxxxxxxxxx.xxxx.xxxxx.x [ 31%]
xxxxxxxxxxxxxxx..xxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxx.xxxxxxxxxxx.xxxxxx.xxxx.xxxxxxxxx.xxxxxxx.x..x.x.x....xx....x.x..x..x........xx..x.x.......x..............................x.....xxx...x...x [ 37%]
x..........xx...................x............x....x......x....x..x.x.xx..x.xx..x...x..x.........x..............x....x.xx..xx.........x..x....x........x..x.........x....x..........xx......... [ 42%]
.....................x...x......xxx...................x......x.....x....x.........x......x..x...x.........x..xx..................x.x.......................x.............x.....x.............. [ 47%]
......x...xx.......x..xx......................................................x...........x.x......x............x.x..........x.............x..x........x..x...x.x.....xx......x......x.......x [ 52%]
..x.......x.x....x.........x.............x......x.....xx..........xx....x........x....x.x...x.........x........................x..........x.....x...............................x...x....x.... [ 58%]
.x...................................x.x....x.....................ss....................................................................................................s........x.xx......... [ 63%]
..s......x...xxxxxxx....x...xxx..x.x...x..x........x.....x......x...........x........xx....xxx.xxx.x.xxxxx.x......x...x...x.........x.x...x..........x......x.....x......x...........x.x...... [ 68%]
.s...x......x..........................x.......x.x.xxx....x........x.x..........x...x.....x.......x...x.x...x...s..xxxx.....x...x.x.xxxx.x..x..xx..x...xxxxx.xx.....xxxxx...x.x..xx.xx....x... [ 74%]
.xxxx.x.......x....xx...xxx.x..xxxxx.x...xxxx...x......x...x.....................x...........x..........xx.................x..........xx......x..x......................x.......xxx........... [ 79%]
...........x.....x......................x........x...x.x.xx.x...x.........x...................xx..x...x...x.........xx.....s...........................................x...................... [ 84%]
.............................................................................................................................................................................................. [ 90%]
...............................x................................................................................................x............................................................. [ 95%]
.................s..s.......................................................................................................x...........................x.............                         [100%]
===================================================================== 2978 passed, 39 skipped, 569 xfailed in 427.64s (0:07:07) ======================================================================

@cpcloud cpcloud requested a review from kszucs April 29, 2024 20:54
@kszucs kszucs mentioned this pull request Apr 29, 2024
Merged
@cpcloud
Copy link
Member Author

cpcloud commented Apr 30, 2024

Closing in favor of #9081.

@cpcloud cpcloud closed this Apr 30, 2024
@cpcloud cpcloud deleted the schemas-are-ordered branch April 30, 2024 11:53
cpcloud added a commit that referenced this pull request Apr 30, 2024
@kszucs @cpcloud
feat(common): introduce FrozenOrderedDict (#9081)
f926995 
Continuation of #9068 by adding
`FrozenOrderedDict` which calculates its hash from `tuple(self.items()`
rather than `frozenset(self.items())` and also checks for item order
during equality checks.

Closes #9063.

---------

Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kszucs kszucs Awaiting requested review from kszucs

Assignees
No one assigned
Labels
internals Issues or PRs related to ibis's internal APIs
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.

bug: .cache() gives incorrect result when column order changes
2 participants
@cpcloud @kszucs