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

cpcloud · 2024-09-15T12:00:38Z

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 · 2024-09-16T10:27:58Z

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 · 2024-09-16T13:12:34Z

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

cpcloud · 2024-09-16T13:30:57Z

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)

ibis/backends/__init__.py

…ables with the same name but different data

jcrist · 2024-09-23T15:53:39Z

ibis/backends/__init__.py

+        # mapping of memtable names to their finalizers
+        self._finalizers = {}
+        self._memtables = weakref.WeakSet()
+        self._current_memtables = weakref.WeakValueDictionary()


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

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

cpcloud added this to the 10.0 milestone Sep 15, 2024

cpcloud added bug Incorrect behavior inside of ibis ux User experience related issues labels Sep 15, 2024

cpcloud force-pushed the memtable-set branch from 0ede913 to 7c69b36 Compare September 16, 2024 10:26

cpcloud force-pushed the memtable-set branch from 7c69b36 to c6c4465 Compare September 16, 2024 11:19

jcrist reviewed Sep 16, 2024

View reviewed changes

ibis/backends/__init__.py Show resolved Hide resolved

cpcloud force-pushed the memtable-set branch 10 times, most recently from 3b1fe38 to aac07cf Compare September 18, 2024 13:47

fix(memtables): track memtables with a weakset to allow overwriting t…

3b38e15

…ables with the same name but different data

cpcloud force-pushed the memtable-set branch from aac07cf to 3b38e15 Compare September 18, 2024 16:20

cpcloud requested a review from jcrist September 23, 2024 09:25

jcrist reviewed Sep 23, 2024

View reviewed changes

cpcloud merged commit bf0a666 into ibis-project:main Oct 8, 2024
75 checks passed

cpcloud deleted the memtable-set branch October 8, 2024 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cpcloud commented Sep 15, 2024

cpcloud commented Sep 16, 2024

cpcloud commented Sep 16, 2024

cpcloud commented Sep 16, 2024

jcrist Sep 23, 2024

cpcloud Sep 23, 2024

cpcloud Sep 26, 2024

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

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

Conversation

cpcloud commented Sep 15, 2024

Description of changes

Issues closed

cpcloud commented Sep 16, 2024

cpcloud commented Sep 16, 2024

cpcloud commented Sep 16, 2024

jcrist Sep 23, 2024

Choose a reason for hiding this comment

cpcloud Sep 23, 2024

Choose a reason for hiding this comment

cpcloud Sep 26, 2024

Choose a reason for hiding this comment