Curious on the removal of SequenceVector support.

[sungho-park-23]

Hello, I saw that the support for SequenceVector was removed from DecodedVector here: d0884ed#diff-75b0b77c87aba61bd1f2237f2a5923dcd7880707e21c6ff3ce192d387541a3bb

I do see that the reason was due to its complexity and lack of testing, but recently I have been finding a use case where RLE-encoding could help -- in the future maybe also be using SequenceVector directly from Parquet files encoded with RLE.

@mbasmanova I see that this commit was by you, do you mind sharing more details on the reasons? Or if there needs to be more tests around it, where would that be? I am open to adding more tests for this.

[mbasmanova]

@sungho-park-23 Thank you for asking. Indeed, I removed support for Sequence and Bias vectors because of the extra complexity they were adding, lack of testing, and lack of evidence that this complexity is justified. I did this after spending many hours debugging and fixing bugs in expression evaluation. It might be difficult to appreciate this challenge without acquiring first hand experience working with the code. If interested, you may want to try fixing a few fuzzer-found bugs. You may find these in https://github.com/facebookincubator/velox/issues?q=is%3Aissue+is%3Aopen+label%3Afuzzer-found

[sungho-park-23]

@mbasmanova Sure! I will look at some fuzzer-found bugs, but also I am working in Bytedance/Tiktok, and have done some investigation in a few areas within velox previously.

I was curious: is the complexity of Sequence and Bias vectors not justified because you find that it does not show real performance gains? We are thinking that the filter and aggregation operators should be able to have some performance gains from SequenceVectors. Is it that during expression evaluation you see that some conditions in project or filter are not really utilizing the RLE encoding of SequenceVector?

[mbasmanova]

DWRF reader does not generate SequenceVectors, and no operator generates these either. Hence, no usage, just complexity in the code.

Also, one can use DictionaryVector instead of SequenceVector.

[sungho-park-23]

Got it, I see. That makes sense! Thanks a lot