Avoid moving DictionaryChunks #2999
Conversation
They now store pointers in the internal cache which would be invalidated when moving and can't be easily updated
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2999 +/- ##
=======================================
Coverage 93.30% 93.31%
=======================================
Files 1124 1124
Lines 42913 42912 -1
=======================================
+ Hits 40041 40043 +2
+ Misses 2872 2869 -3 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| inline DictionaryChunk& getDictionaryChunk() { return dictionaryChunk; } | ||
| inline const DictionaryChunk& getDictionaryChunk() const { return dictionaryChunk; } | ||
| inline DictionaryChunk& getDictionaryChunk() { return *dictionaryChunk; } |
There was a problem hiding this comment.
I'm thinking to form a convention of naming functions returning non-const reference with suffix UnSafe (or maybe other suffix to be more explicit). What do you think? Is this necessary?
|
Do we have a minimal example to reproduce this? so we can add to our test suite. |
|
@benjaminwinger I'm merging it so we can ship to for today's nightly build. |
|
I'll see if I can come up with an example. It's caused by accessing strings after |
There was a regression in #2994 which wasn't caught by our test suite. On some datasets it was causing a segfault when copying rel tables.
DictionaryChunknow stores pointers in its internal cache which would be invalidated when moving and can't be easily updated (which is understandable, changing the hash function and equals functions in an unordered_set usually would be expected to break things, even if in this case it would be safe).