fix(cpp): handle TS2DIFF float prefixes in batch decode - #901
Conversation
|
Thanks for tracking this down. The root-cause analysis is clear, and the new implementation correctly handles Java-compatible prefixes, including overflow prefixes and reads spanning multiple segments. I found one blocking compatibility issue, though: routing FLOAT/DOUBLE batch reads through the scalar decoder regresses legacy raw segments. The scalar prefix detector can misclassify a valid raw header, after which the decoder gets an invalid bit_width_ and spins at end-of-input. I reproduced this for both FLOAT and DOUBLE by encoding 129 sequential raw bit patterns with IntTS2DIFFEncoder / LongTS2DIFFEncoder, then reading them in small batches through the corresponding floating-point decoder. The PR head hangs, while the parent implementation completes successfully. Could we preserve the integer batch path for legacy raw segments, or make the prefix detection unambiguous before switching to the scalar path? It would also be good to add legacy raw batch regression tests for both types. |
ColinLeeo
left a comment
There was a problem hiding this comment.
The overall fix direction looks good, but the legacy raw segment compatibility issue is not fully addressed yet.
Summary
Root cause
The FLOAT/DOUBLE batch overrides delegated directly to the INT32/INT64 TS_2DIFF batch decoders and then bit-cast the results. Those integer decoders expect a delta-block header at the current stream position, but FLOAT/DOUBLE segments place a
maxPointNumberor overflow-bitmap prefix before that header. The prefix was therefore decoded as block metadata, leaving the stream and decoder state inconsistent. In tree-reader batch paths this could result in unbounded decoding at end-of-input.The scalar
read_floatandread_doubleimplementations already consume the prefix and apply scale/overflow handling. Reusing those implementations restores correctness for normal, overflow, and legacy raw segments. This intentionally gives up the integer SIMD fast path for FLOAT/DOUBLE until a prefix-aware optimized implementation is available.Tests
FloatDoubleTS2DIFFCodecTest.*TS2DIFFCodecTest.*FloatTS2DIFFEncoderResetTest.*EncodingCoverage.TS2DIFF*TreeQueryByRowTest.QueryByRow_TabletMultiType_PartialPathsTsFileWriterTest.WriteDiffrentTypeCombinationclang-format --dry-run --Werror cpp/src/encoding/ts2diff_decoder.h cpp/test/encoding/ts2diff_codec_test.ccFixes #900