Skip to content

Conversation

@kkrik-es
Copy link
Contributor

@kkrik-es kkrik-es commented Nov 10, 2025

Existing bit-packing logic is customized to apply to blocks of 128 longs. For larger blocks that are multiple of 128, the bit-packing logic is extended to be applied to blocks of size 128 iteratively. This is expected to be a no-op, since the codec block size is 128, but unlocks experimenting with larger blocks.

Related to #136307

@kkrik-es kkrik-es marked this pull request as ready for review November 10, 2025 13:34
@kkrik-es kkrik-es added the test-full-bwc Trigger full BWC version matrix tests label Nov 10, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Contributor

@parkertimmins parkertimmins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been looking at the numeric compression lately, so went ahead and gave this a review. 😃
Just a couple of nits, logic looks correct.

private static final int BITS_IN_SIX_BYTES = 6 * Byte.SIZE;
private static final int BITS_IN_SEVEN_BYTES = 7 * Byte.SIZE;

private static final int BITBACK_BLOCK_SHIFT = 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BITBACK -> BITPACK?

assert numericBlockSize >= BITBACK_BLOCK_SIZE && (numericBlockSize & (BITBACK_BLOCK_SIZE - 1)) == 0
: "expected to get a block size that a multiple of " + BITBACK_BLOCK_SIZE + ", got " + numericBlockSize;
this.blockSize = numericBlockSize;
this.encoded = new byte[numericBlockSize * Byte.SIZE];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It be the same value, but would be clearer ifByte.SIZE were Long.BYTES`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants