fix: disallow wrapping auto-detected fsst in other compression#6120
fix: disallow wrapping auto-detected fsst in other compression#6120westonpace merged 1 commit intolance-format:mainfrom
Conversation
Signed-off-by: Daniel Rammer <hamersaw@protonmail.com>
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
PR ReviewBug fix is correct. The logic change gating auto-FSST on compression.is_none() cleanly prevents zstd(fsst(data)) when the user explicitly sets a compression scheme. Missing test coverage (P0): Per project guidelines, all bugfixes must have corresponding tests. This PR has no test that verifies the fix. A test that sets compression to zstd on a Utf8 field with FSST-candidate data and asserts that FsstMiniBlockEncoder is NOT present in the resulting compressor would directly cover this bug. Minor note: The fix to create_per_value (line 628) is technically unreachable for the reported bug scenario. When compression is set to zstd, the early return at line 620-623 (per_value_requested) already diverts to CompressedBufferEncoder before reaching use_fsst. Fixing it for consistency is reasonable, but worth noting it was not the active bug path. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Currently, if a user manually sets a compression type (ex. zstd) we can inject FSST compression underneath. Resulting in
zstd(fsst(data))rather thanzstd(data)that the user intended. This PR updates our auto-injection of FSST to only occur if the user has not specified a compression type manually.Related to #5248