Fix compressed spooling: compare decoded length against integer segment sizes#624
Open
arpitjain099 wants to merge 1 commit into
Open
Fix compressed spooling: compare decoded length against integer segment sizes#624arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Signed-off-by: Arpit Jain <arpitjain099@gmail.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.
Description
CompressedQueryDataDecoder.decodeguards the compressed spooling path by checking the segment length against the sizes the coordinator reports in the segment metadata. Those sizes arrive as strings (the_SegmentMetadataTOTypedDict typessegmentSizeanduncompressedSizeasstr, and the metadata built across the spooling tests uses string values like"10"), but the code compared them directly tolen(data), which is an int. An int is never equal to a decimal string, sonot len(data) == metadata["segmentSize"]is always true and every compressed segment raisedRuntimeErrorbefore it was ever decompressed. The error text gives it away, it reads "Expected to read 29 bytes but got 29" and still raises.This makes json+zstd and json+lz4 spooling unusable from the client even though both encodings are advertised in the
X-Trino-Encodingheader. The lz4 path already coerces the size withint(...)when callinglz4.block.decompress, so the two size checks here just needed the same treatment.The fix wraps both metadata sizes in
int()before comparing, matching the existing lz4 code. I also added a unit test that decodes a real zstd-compressed segment with the string-typed metadata the protocol actually sends. It fails against the old code with the "expected N, got N" RuntimeError and passes with the fix. The existing spooling tests missed this because they all decode withencoding="json"or mock the decoder, so the compressed decode path had no coverage.Non-technical explanation
Compressed results returned through the spooling protocol were always rejected with an error. This fixes the size check so compressed segments decode correctly.
Release notes
(x) Release notes are required, with the following suggested text: