Stateless Row Encoding / Don't Preserve Dictionaries in RowConverter (#4811)
#4819
Conversation
github-actions
bot
added
arrow
|
FYI @sunchao I recall you mentioning the dictionary encoding overheads were very high in your workloads |
|
I also added some additional rationale to the top of this PR to explain the CPU vs memory tradeoffs to try and help better motivate this change |
alamb
changed the title
RowConverter (#4811)
There was a problem hiding this comment.
Given all our benchmarks go faster and how many problems we have had with dictionary interning I am highly supportive of this approach.
I had a few benchmark questions, but otherwise 🏆
I looked at the benchmark results -- do we have any coverage for convert "low" cardinality dictionaries? The results posted seem to show that for high cardinality dictionaries (aka where each value is a random value) things get much faster, as expected
FYI @JayjeetAtGithub if we merge this I think apache/datafusion#7401 might not be needed
However, I still think getting some end to end performance numbers to find where preserving dictionaries improves performance (if at all) would still be hugely valuable to quanity the end to end performance effect that removing the dictionary interning from the row converter would hanve
This is a very good point, added some benchmarks with "low" cardinality dictionaries containing 10 unique values. The performance gain from this PR is reduced, but is still sizeable, especially in the case where the values haven't been seen before. Edit: actually messed up, standby |
tustvold
force-pushed
the
stateless-row-encoding
branch
from
47a8e43 to
daa31df
Compare
tustvold
force-pushed
the
stateless-row-encoding
branch
from
daa31df to
b864c5e
Compare
Thanks @tustvold ! Yes, we observed the columnar-to-row conversion for dictionary encoding incurs quite some overhead. Even in my current vectorized hash table POC, which is based of |
|
Now that I got the benchmarks working correctly they make a lot more sense 🤦... With a low cardinality dictionary consisting of 10 distinct values, the method in this PR still yields a performance advantage with moderate-sized strings of less than 30 bytes. This relationship inverts once we get to strings consisting of 100 bytes, with a non-trivial regression for strings of this size. This is inline with my expectations, in the ideal case of a small dictionary containing large strings, the interning logic does represent a benefit, I'm not sure how common this case is |
Which issue does this PR close?
Closes #4811
Rationale for this change
Interning of
DictionaryArrayvalues is slightly faster forStringdictionaries with a small number of distinct values, as the representation inRowformat is more concise than copying the actual underlying value. However, as we have discovered several times in downstream projects (for example apache/datafusion#7200 from @JayjeetAtGithub ) the interning has huge downsides once the number of distinct keys becomes "large". Specifically:I initially considered a more holistic rework of the API to entirely remove
RowConverter, but it quickly became quite a large change. Instead this PR starts small and simply removes the dictionary interning, allowing the RowConverter methods to take&selfinstead of&mut self. This will simplify sharing aRowConverteracross multiple threads, e.g. by wrapping it inArc.There is more discussion on #4811
What changes are included in this PR?
Removes the dictionary preservation logic from RowConverter.
It isn't exactly a fair comparison, but the performance improvements are very significant.
Are there any user-facing changes?
This disables dictionary preservation for keys, leading to different output types and memory / performance characteristics.
#4813 tracks potentially preserving the dictionary on decode, but I'm not yet sure about the benefits/practicalities of doing this.
#4712 tracks potential further performance improvements to this dictionary encoding approach