feat: BitPackedCompressor allows signed arrays

[a10y]

Most of the work to support signed integers has been done in BitPackedArray.

This PR removes some assertions and branches in the compressor to make it possible to bit-pack an array of signed ints.

[gatesn]

Is it worth lumping in the change to disallow a no-op FoR array too?

Or better yet, disallow FoR when it has minimal impact on bitpacking width.

[a10y]

I've changed the condition in FoRCompressor::can_compress to now always reject if min=0 and shift=0. Previously if the type was signed it would not bail.

I did not change FoRCompressor to determine how many bits of savings is enough to make up for the extra unfor step. I don't have a good intuition on what that would be.

[lwwmanning]

I've changed the condition in FoRCompressor::can_compress to now always reject if min=0 and shift=0. Previously if the type was signed it would not bail.

I did not change FoRCompressor to determine how many bits of savings is enough to make up for the extra unfor step. I don't have a good intuition on what that would be.

I can live with excluding if min and shift are zero (as a way to short-circuit), but it's a bit unnecessary. The sampling compressor already penalizes extra array depth. So FoR(0,0) + Bitpacking on signed ints will be strictly worse than just Bitpacking and won't be chosen anyway

[gatesn]

I've changed the condition in FoRCompressor::can_compress to now always reject if min=0 and shift=0. Previously if the type was signed it would not bail.
I did not change FoRCompressor to determine how many bits of savings is enough to make up for the extra unfor step. I don't have a good intuition on what that would be.

I can live with excluding if min and shift are zero (as a way to short-circuit), but it's a bit unnecessary. The sampling compressor already penalizes extra array depth. So FoR(0,0) + Bitpacking on signed ints will be strictly worse than just Bitpacking and won't be chosen anyway

By this logic we shouldn't exclude any compressors based on metrics, e.g. run-end encoding should ignore the avg_run_length.

[lwwmanning]

I've changed the condition in FoRCompressor::can_compress to now always reject if min=0 and shift=0. Previously if the type was signed it would not bail.
I did not change FoRCompressor to determine how many bits of savings is enough to make up for the extra unfor step. I don't have a good intuition on what that would be.

I can live with excluding if min and shift are zero (as a way to short-circuit), but it's a bit unnecessary. The sampling compressor already penalizes extra array depth. So FoR(0,0) + Bitpacking on signed ints will be strictly worse than just Bitpacking and won't be chosen anyway

By this logic we shouldn't exclude any compressors based on metrics, e.g. run-end encoding should ignore the avg_run_length.

Yeah, I think that's correct. More importantly, we should also filter codecs based on whether can_compress the full array, THEN do the sampling compression, then compress-like the best on sample. Right now, we check can_compress on the sample, which may fail on the full array, causing a resample... which could fail arbitrarily many times.

E.g., if we theoretically implemented BitPackedArray::can_compress for signed ints by checking if min >= 0 on an array with 1 million integers, of which only 1 was negative, we'd expect it to have to resample ~1000 times on average in order to successfully compress.