fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) - #901
fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910)#901kkzi wants to merge 8 commits into
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
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.
The per-block heuristic that distinguished Java-compatible maxPointNumber prefixes from legacy raw delta blocks could misclassify a valid raw header (wi = 0 or bit_width = 0 blocks), which desynced the stream and could spin at end-of-input in batch reads. Decide the page layout once per page instead: parse the whole remaining stream with the Java segment grammar (prefix + overflow bitmaps + block run, validated field ranges and exact exhaustion) and cache the segment prefix offsets. A legacy raw page fails this parse because its first misaligned write_index probe reads >= 0x100. - Legacy raw pages keep the integer SIMD batch decode path with bit-cast semantics (parent-commit behavior). - Java pages consume prefixes only at recorded offsets and take the segment-aware scalar path; this also fixes value semantics across blocks inside one Java segment, which the per-block heuristic could not represent. - Bail out of read_long() when the stream is exhausted with bits still owed, so no residual misconfiguration can loop forever. Also fix ByteStream::check_space(): after set_read_pos() parks the cursor at a page boundary, blindly following read_page_->next_ skipped the boundary page and failed reads with E_OUT_OF_RANGE. Recompute the page from the head instead; page chains are short so the walk is cheap. Add legacy raw batch/scalar/mixed regression tests for FLOAT and DOUBLE (PR apache#901 review).
|
Hi @ColinLeeo, thanks for the thorough review and the reproduction steps — they made this straightforward to chase down. I've pushed Root cause confirmed. Your repro hangs exactly as described: the per-block heuristic ( Fix — unambiguous prefix detection (your option 2). The layout is now decided once per page by Legacy raw batch path preserved (your option 1). Legacy raw pages route through the integer SIMD batch decoder + bit-cast, exactly the parent-commit behavior; Java pages take the segment-aware scalar path. Regression tests. Added for both FLOAT and DOUBLE:
Also hardened One incidental fix this surfaced: Full C++ suite (757 tests) passes, and |
kkzi
changed the title
…apache#910) Root cause of apache#910: the C++ FloatTS2DIFFEncoder/DoubleTS2DIFFEncoder wrote the maxPointNumber field (fixed value 2) at every segment boundary, while Java FloatEncoder/DoubleEncoder write it only once at the start of each page. Files written with an empty/short first segment could then be misparsed by Java readers (e.g. TsFileSketchTool crashing on the trailing maxPointNumber). This change aligns the C++ encoder with the Java layout: - Encoder: the maxPointNumber var_uint is now emitted exactly once per page (on reset, before segment 1). Segment boundaries only carry the overflow/underflow FLAG when needed, matching Java's segment grammar. - Decoder: forward-only, prefix-aware parsing that accepts all three page layouts — legacy raw pages (no prefix at all), the new Java format (maxPointNumber only on the first segment), and old C++ per-segment format (backward compatible). The old peek-and-rewind scheme is gone; the segment header of a prefix-free segment is preloaded so decode() never needs to re-read the stream. - Tests: new gtest cases assert the maxPointNumber-once-per-page byte layout for multi-segment pages, scaled-overflow pages (the apache#910 crash scenario), reset() page boundaries, and legacy per-segment backward compatibility. Verified: full C++ test suite passes; Java TsFileSketchTool reads files written by the fixed encoder; tsfile_cli round-trips the data.
|
Thanks for the review. I have pushed The CI runs for this new commit are currently waiting for approval ( Happy to address any remaining feedback on the legacy-raw compatibility path. |
The pin (a66a679) carries 4 TS_2DIFF float/double fixes not yet merged upstream (PR apache/tsfile#901 open); the branch lives only in the kkzi/tsfile fork. Clones resolving the pin need that fork reachable: git submodule update --init 3rd/tsfile # may fail on the pin git -C 3rd/tsfile remote add fork git@github.com:kkzi/tsfile.git git -C 3rd/tsfile fetch fork a66a6796 git -C 3rd/tsfile checkout a66a6796 Once #901 merges, bump the pin to upstream develop and drop this note.
The pin (a66a679, TS_2DIFF float/double fixes, PR apache/tsfile#901 open) only exists on the fork's fix branch, so the fork is the canonical source until the PR merges. branch = fix/cpp-ts2diff-float-double-batch-prefix. Verified end-to-end: files written by this pin's writer decode correctly through IoTDB 2.0.10's Java tsfile lib (tsfile-2.3.1).
…atch) CRT ::open interprets bytes in the active code page; UTF-8 paths with non-ASCII characters fail with E_FILE_OPEN_ERR (28) on machines where the 8.3-shortpath / ACP-transcode workarounds unavailable (8dot3 disabled on the volume, or ACP cannot represent the characters). file_internal::open_utf8 converts UTF-8 -> wide chars -> _wopen, same as the vendored TsFileCpp tree. Applied to ReadFile::open, WriteFile, and RestorableTsFileIOWriter's self-check reader.
…iter windows.h from utf8_file_open.h before decoder_factory.h made INT32/ DATE/DOUBLE ambiguous with using-namespace common in the decoder switch.
get_timeseries_schema built MeasurementSchema with the 2-arg ctor, whose encoding/compression are library defaults (DOUBLE->GORILLA, LZ4) rather than what the file stores. Take both from the first ChunkMeta of the timeseries (chunk metadata is deserialized from the file), falling back to defaults when no chunk metadata is available.
… bytes ChunkMeta entries from the metadata index carry only offsets (C++ deserialization never fills encoding_/compression_type_, unlike Java), so the previous attempt read uninitialized memory. Now: read 256 bytes at the first chunk's offset_of_chunk_header_ and deserialize the ChunkHeader (encoding/compression live there). Adds TsFileIOReader::get_read_file().
2 participants
Fix C++ TS_2DIFF float/double encoding to match the Java layout, and make the decoder tolerate all three page layouts. Fixes #910.
Summary
FloatEncoder/DoubleEncoder. The Java readers (e.g. TsFileSketchTool) crash on the old layout when a page's first segment is empty/short — that is fix(cpp): Float/DoubleTS2DIFFEncoder writes maxPointNumber per segment, breaking Java FloatDecoder on multi-segment pages #910. The encoder writes maxPN at page start (first encode after reset), so every non-empty page begins with either a FLAG section or the maxPN prefix — this is the invariant the decoder relies on.decode()never rewinds the stream.scan_java_float_double_pagein 1ef5e94). The scan assumed every segment carries a prefix and could not bound segment boundaries on new-format pages (segments 2+ have no prefix and no separator), which made scaled-overflow pages (FLAG + prefix-free continuation) unreadable. The forward-only parser dispatches on the first byte (0x00 / 0x02 / FLAG) with a per-pagepage_first_segment_flag, which handles the new layout unambiguously.check_space()recomputes the read page from the head instead of blindly followingnext_when the cursor is parked at a page boundary (from 1ef5e94). The decoder's probe rewinds (set_read_posfallback branches) rely on this.reset()page boundaries, and legacy per-segment backward compatibility. Legacy raw batch/scalar/mixed regressions from the earlier review are kept.Verification
TsFileSketchToolreads files written by the fixed encoder (previously crashed).tsfile_cliround-trips the data.