Fix remaining_length accounting after Huffman decode in RegressionPredictor / ComposedPredictor - #137
Open
alexey-milovidov wants to merge 1 commit into
Conversation
…mposed predictors
RegressionPredictor::load decremented remaining_length by the uncompressed
index count (coeff_size * sizeof(int)) after Huffman decode(), but decode()
advances the read pointer only by the compressed stream size. The uncompressed
count overshoots the compressed stream, understating remaining_length for the
subsequent quantizer encoder.load(), whose bound check then spuriously rejects
valid data with "SZ3 Huffman: tree exceeds compressed buffer".
RegressionPredictor is only used by ALGO_LORENZO_REG, so a column compressed
with CODEC(SZ3('ALGO_LORENZO_REG', ...)) could be written on insert but fail
every subsequent read with CORRUPTED_DATA (effective data loss). Small element
counts are affected; large counts happen to leave enough slack to survive the
understated bound.
Account for exactly the bytes decode() consumed via pointer difference.
ComposedPredictor::load had the mirror defect (it never decremented
remaining_length for its selection stream); tightened the same way.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RegressionPredictor::loadthreads aremaining_lengthbyte counter through its sub-reads so later reads know how many bytes are left. After Huffman-decoding the regression coefficients it does:decodeadvances the read pointercby the compressed (Huffman) stream size, butremaining_lengthis decremented bycoeff_size * sizeof(int)— the uncompressed index count. Those two are unrelated, so after this pointremaining_lengthno longer reflects the bytes actually consumed (it typically overshoots the compressed stream and understates the remainder, and can even wrap past zero).ComposedPredictor::loadhas the mirror defect: it Huffman-decodes a per-block predictor-selection stream but never decrementsremaining_lengthfor it at all.Today the
readhelpers inMemoryUtil.hpponly guard the counter withassert, so a release build tolerates the wrong value and the bug is latent. But any reader that actually enforces the bound will then reject valid data. This surfaced while integrating SZ3 into ClickHouse, whose hardened build makes those reads throw instead ofassert: a column compressed withALGO_LORENZO_REGcould be written but failed every subsequent read.Fix
Decrement
remaining_lengthby exactly the bytesdecodeconsumed, measured as the pointer advance:and apply the same accounting to the selection stream in
ComposedPredictor::load. Behavior for valid data is unchanged; the counter now tracks the real byte cursor.Context
Found while integrating SZ3 into ClickHouse. Companion fix on the ClickHouse fork: ClickHouse#2