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.

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

fix(memtables): track memtables with a weakset to allow overwriting tables with the same name but different data #10133

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Sep 15, 2024

Description of changes

Fixes an issue with memtable name reuse where we were never overwriting
memtable data in the backend, even if the data changed and the name remained
the same.

The solution I came up with here was to add a weakref.WeakSet to track
memtable ops, which are hashed based on the data-containing object.

The reason a WeakSet is necessary is to preserve the drop-on-unreachable
semantics of memtable.

Issues closed

Closes #10131.

@cpcloud cpcloud added this to the 10.0 milestone Sep 15, 2024
@cpcloud cpcloud added bug Incorrect behavior inside of ibis ux User experience related issues labels Sep 15, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Sep 16, 2024

I know that previously we decided against using con.list_tables() but I think it reflects the specific UX we want for memtables here, and avoids mucking around in implementation details of how exactly memtables are stored, collected and dropped.

@cpcloud
Copy link
Member Author

cpcloud commented Sep 16, 2024

That said, I think we'll want to adjust this test to use list_temp_tables() once that lands.

@cpcloud
Copy link
Member Author

cpcloud commented Sep 16, 2024

Clouds passing:

…/ibis on  memtable-set is 📦 v9.5.0 via 🐍 v3.12.5 via ❄️   impure (ibis-3.12.5-env)
❯ pytest -m bigquery -n auto --dist loadgroup --snapshot-update -q && pytest -m snowflake -n 8 --dist loadgroup --snapshot-update -q
bringing up nodes...
..........s.s.............s...............s...........x.x.................sssssssssssssssssssss.ssssssssssssssss [  5%]
sssssssssssssssssssssssssssssssssssssssssss.sssssssssssssssssssssssssssssssssssssssss.......x..............xx... [ 10%]
.......s..x...............................s......................x.............................................. [ 15%]
..............x...................xx....x............x..........x.............................x.xxxx..x..xxx.x.. [ 20%]
...xx.xx.x.x.x.xxx.xx....xxx.xx.xxxxx.x.x..xx.x.x.....xx..........x...x..........x..x......x...xx.x....xx.x....x [ 25%]
..x.........x..........x.......x.....x..............xx.x......................x..............x...xx.....x...x... [ 30%]
.....x......x..........x...................x.......x........................x.x............x...x..x.......x..... [ 35%]
....................................sx...............x.x..x..s...............s.......s......s.x.......x......... [ 40%]
.x.....x..........x..x.....xx...x.....x......x..x........................x..x......x.x...........xx...xx...x.... [ 45%]
....x.x......xx.X........x...x...x....x.x.....x....x.x..x....x.........x.x..X...xx.x.xXx.x..x......xx..xx....... [ 50%]
xx..x........x.........xX.x......x....xx.........................x......x....x......xx..............xx.x.x....xx [ 55%]
..x.x..x....x........x.....x....xx..x.x.....................x.xxx..xx......x...x....x.xxxxxxxx.xxxxxxx..xxxxx.xx [ 61%]
xxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxxxxxx.xxxxxx.xxx.xxxxxxxxxxxx.x.xxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxxxxxx....x..x [ 66%]
.xx.x............x.............x..................................x.x.....x.xx..........................x.....x. [ 71%]
..............x.......xxxxxxxxxxxx....................x.......................x..x........x.x.x..x.............. [ 76%]
......x..............................x...x........x.....x...x..xx.x......x...................x.................. [ 81%]
.................x..........x...........x....................................................................... [ 86%]
...............................................................x................................................ [ 91%]
................................................................................................................ [ 96%]
..x.............x.........................................................                                       [100%]
1715 passed, 132 skipped, 351 xfailed, 4 xpassed in 395.85s (0:06:35)
bringing up nodes...
......xxx...xx.x.....xxx.x.x.xxxx...x..........................................x.....x.......................... [  5%]
..................................x.x....................x.x.......x.......x..x......x...............x....xxx.x. [ 11%]
.....................x..x.............x...x......xx....x...x..............................x..................x.. [ 17%]
....................................x..x........xx........................x....x..........x.......xx............ [ 22%]
x....x.....xx.........................x....................x.x.x...................................x............ [ 28%]
................xx.......xx...x...xx.....x.........s...............x...........................................x [ 34%]
..........................x..................x.......x....................x.........x..........x................ [ 39%]
......................................x.............................x..x.xx.x.x.xx....x..xx.....x..x.xxx...xxx.x [ 45%]
x..xxx.xx.x.xx.x..xx.......x.x.xx..x...x..x.x..xxxxx..x.x..xx.....xxx.x.xxxxx.x....xx...x........x...x.......... [ 51%]
......x...x..............x....x.......x.............x.....xx......x..................xxx.....x....x............. [ 56%]
........x....x..........x...x.......xx..x..x..x.........x...x.x..x..x.............x............................. [ 62%]
..x.......xx.x..........................................................................s........x.........x.... [ 68%]
...x...x.........................................x.......x.....xx....x.......x....xx..................xxx..xxx.. [ 73%]
...x.....x..x.x...x..x..x.......x..........x.x....x..xx..x.xx.x..x.x..x............x.....x................x..s.. [ 79%]
s.............................................................s..................x.............s....x........... [ 85%]
..................xxxxxxxx.x.x.x...x.........................................x.........x..................s..... [ 90%]
......s......................................................................................................... [ 96%]
....................................................s..............s.                                            [100%]
1733 passed, 10 skipped, 230 xfailed in 200.23s (0:03:20)

@cpcloud cpcloud force-pushed the memtable-set branch 10 times, most recently from 3b1fe38 to aac07cf Compare September 18, 2024 13:47
# mapping of memtable names to their finalizers
self._finalizers = {}
self._memtables = weakref.WeakSet()
self._current_memtables = weakref.WeakValueDictionary()
Copy link
Member

Choose a reason for hiding this comment

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

I think we can reduce this state to just a single weakset and dict, mind if I push up a commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Go for it!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcrist I am still open to you pushing up a commit if you've already got it locally, otherwise, no big deal.

@cpcloud cpcloud merged commit bf0a666 into ibis-project:main Oct 8, 2024
75 checks passed
@cpcloud cpcloud deleted the memtable-set branch October 8, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(memtable): reusing same name for new memtable does not overwrite in REPL
2 participants