Fix compressed spooling: compare decoded length against integer segment sizes - #624
Fix compressed spooling: compare decoded length against integer segment sizes#624arpitjain099 wants to merge 1 commit into
Conversation
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
|
Thanks for digging into this. I tried to reproduce the failure before merging and couldn't get it to happen against a real coordinator. Were you able to reproduce this? I checked out this branch reverted just the int(...) casts to get back to the old behavior and ran it against a coordinator with spooling and json+zstd/json+lz4 enabled. I didn't see any failures. I also printed type(metadata["segmentSize"]) directly in the decoder and it came back as int, not str. I think the existing int(...) cast in There might be a bug in trino/client.py has ignore_errors = true in mypy config so nothing catches this mismatch. We should instead fix the annotation and the test fixtures directly instead of adding casts around it. |
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: