fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910)

[kkzi]

Fix C++ TS_2DIFF FLOAT/DOUBLE encoding to match the Java layout, and make the decoder accept every layout the format admits. Fixes #910.

Summary

Encoder — write the maxPointNumber var_uint exactly once per page instead of at every segment boundary, matching Java FloatEncoder/DoubleEncoder. Java readers (e.g. TsFileSketchTool) crash on the old layout when a page's first segment is empty or short: that is #910. maxPN is emitted at page start (first encode after reset), so every non-empty page begins with either a FLAG section or the maxPN prefix — the invariant the decoder relies on.

The default is 2, not 0: TSEncodingBuilder.Ts2Diff initializes 0, but the standard schema write path goes through initFromProps(), which substitutes the max_point_number property or TSFileConfig.floatPrecision (default 2). set_max_point_number() is added so a non-default precision can be encoded; nothing in the C++ write path calls it yet (the encoder factory does not read schema props), so it is currently exercised only by the tests that pin the mpn = 0 and mpn = 1000 layouts.

Decoder — forward-only, prefix-aware parsing that accepts legacy raw pages (no prefix, first byte 0x00), the Java layout (maxPointNumber only on the page's first segment), and the older C++ per-segment format. Page-wide metadata (maxPointNumber, value count, overflow bitmaps) is parsed once per page and applies to every block in it; a prefix-free segment's header is preloaded so decode() never rewinds.

Two bounds that had no basis in the format were removed:

• maxPointNumber > 100 — Java accepts any varint the stream carries (Math.pow overflow yields +inf, and those values then take the raw-bits form). MaxPointNumberAboveLegacyBoundDecodes covers mpn = 1000.
• writeIndex > 128 — 128 is DeltaBinaryEncoder.BLOCK_DEFAULT_SIZE, a buffer size, and Java exposes block-size constructors. read_header now bounds writeIndex by stream availability, plus by the page value count when the page metadata supplies one (a zero-width block occupies no packed bytes, so availability alone cannot bound it). Both bounds use int64 arithmetic — writeIndex * bitWidth and writeIndex + 1 each overflow int32 at the extremes. LargeBlockBeyondDefaultSizeDecodes covers a 200-value block through the scalar and batch paths; RejectsBlockBeyondPageValueCount pins the zero-width case.

Rounding — convert_float_to_int/convert_double_to_long follow Java Math.round semantics (floor(x + 0.5), ties toward +infinity) rather than std::lround (ties away from zero), so -0.125 * 100 = -12.5 stores as -12 and matches the Java writer byte for byte. The conversion saturates at the integer limits, including the 2^63 boundary where a plain static_cast is UB.

Error propagation — the read paths used to discard the return of read_i32/read_i64 for delta_min/first_value, and read_int32/read_int64/read_float/read_double returned E_OK unconditionally, so a truncated page surfaced as plausible-looking values instead of an error. Failures now latch and propagate out of the read_* entry points.

MeasurementSchema::deserialize_from — a pre-existing bug found while validating the mpn0 fixture, unrelated to TS_2DIFF but on the path this PR exercises: the props loop iterated props_.size() (always 0 there) instead of the deserialized count, so Java-written (key, value) props pairs were never consumed off the stream and desynced the following TsFileMeta bloom filter read (E_TSFILE_CORRUPTED). Any Java-written file whose schema carries props hit this.

Tests

• cpp/test/encoding/ts2diff_codec_test.cc: once-per-page byte layout for multi-segment pages, scaled-overflow pages (the fix(cpp): Float/DoubleTS2DIFFEncoder writes maxPointNumber per segment, breaking Java FloatDecoder on multi-segment pages #910 crash scenario), reset() page boundaries, legacy per-segment and legacy raw batch/scalar/mixed regressions, mpn = 0 / 2 / 1000, large blocks, truncated pages (headers, fixed fields, mid-block), and the Java rounding ties.
• Cross-language compat matrix extended on both sides with FLOAT/DOUBLE TS_2DIFF and 300-row fixtures (300 rows crosses the 129-value block boundary: 129+129+42). The C++ generator asserts the codec actually recorded in the chunk header, so a fixture cannot pass by silently falling back to a different encoding.
• *.mpn0 fixtures carry an explicit max_point_number=0 property to exercise the canonical 0x00 page prefix. These are Java-generated and C++-validated only — the C++ generator does not emit mpn0 cases, since the C++ writer has no schema-props path to request a non-default precision.
• cpp/docs/ts2diff-float-double-wire-format.md documents the canonical layout derived from the Java reference implementation.

Verification

• Full C++ suite: 805 passed / 3 skipped of 808. The 3 skips are env-gated by design (2 compat fixture entry points plus the external dataset index).
• Java TsFileSketchTool reads files written by the fixed encoder (previously crashed); tsfile_cli round-trips the data.
• clang-format 17.0.6 clean.
• LZMA2 compat cases were not covered locally — this build has ENABLE_LZMA2=OFF because of the separate known Windows/MSVC issue on develop — and are left to CI.

[ColinLeeo]

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]

The overall fix direction looks good, but the legacy raw segment compatibility issue is not fully addressed yet.

[kkzi]

Hi @ColinLeeo, thanks for the thorough review and the reproduction steps — they made this straightforward to chase down. I've pushed 1ef5e94 addressing all three points.

Root cause confirmed. Your repro hangs exactly as described: the per-block heuristic (looks_like_ts2diff_header) only validated a single misaligned block header (wi/bw range check). A legacy raw block with wi = 0 (or bit_width = 0, i.e. any constant-value block) passes that probe with all-zero bytes, gets misread as a maxPointNumber prefix, and the desync cascades into the end-of-input spin.

Fix — unambiguous prefix detection (your option 2). The layout is now decided once per page by scan_java_float_double_page(), which parses the entire remaining stream with the Java segment grammar: [overflow flag][count][bitmaps][mpn] block+, with field-range validation, Σ(wi+1) == bitmap count for overflow segments, and exact whole-stream exhaustion. A page only counts as Java-compatible when the grammar consumes it exactly. A real legacy raw page fails this immediately: after the varint tag eats the leading 0x00, the misaligned write_index probe reads >= 0x100 and is rejected. The detected prefix offsets are recorded and the decoder only consumes prefixes at those offsets, which also fixes value semantics for Java single-segment multi-block pages the per-block heuristic couldn't represent.

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:

• ReadBatchFloatLegacyRawSegments / ReadBatchDoubleLegacyRawSegments — 129 values via IntTS2DIFFEncoder/LongTS2DIFFEncoder (128-value constant block + trailing change, hitting both the bit_width = 0 and wi = 0 misclassification patterns), read in small batches of 16
• ReadFloatLegacyRawScalar / ReadDoubleLegacyRawScalar — scalar path across the block boundary
• LegacyRawBatchThenScalarReads — mixed batch/scalar reads on one page

Also hardened read_long() to bail out when the stream is exhausted with bits still owed, so no residual misconfiguration can loop forever.

One incidental fix this surfaced: ByteStream::check_space() skipped a page when set_read_pos() parked the cursor at a page boundary (it blindly followed read_page_->next_, yielding E_OUT_OF_RANGE on the next read). The scan-based detection depends on position restore, so I fixed it to recompute the page from the head.

Full C++ suite (757 tests) passes, and clang-format --dry-run --Werror is clean. Happy to adjust if you'd prefer a different split.

[kkzi]

Thanks for the review. I have pushed a66a679 which fixes the spotless clang-format violations flagged by CI (ts2diff_decoder.h and ts2diff_codec_test.cc).

The CI runs for this new commit are currently waiting for approval (action_required) — could you approve the workflows so they can re-run?

Happy to address any remaining feedback on the legacy-raw compatibility path.

[ColinLeeo]

One more thought regarding the compatibility design: do we really need to support the historical C++ TS_2DIFF layout?
Since Java implementation is the reference for TsFile format compatibility, I think the previous C++ behavior (writing maxPointNumber for every block) should be considered as a C++ implementation bug rather than a legacy format that needs to be preserved.
Supporting this old layout introduces additional format detection logic and ambiguity in the decoder. It may make the implementation harder to maintain.
Would it be possible to simplify this PR by:

• making C++ writer follow the Java layout exactly;
• making C++ reader support the Java layout only;
• removing the old C++ layout compatibility path?

Then the regression tests can focus on Java ↔ C++ interoperability.

[ColinLeeo]

Hi, @kkzi

Clarification of the FLOAT/DOUBLE TS_2DIFF Format and the Direction of This Fix

TL;DR

• The Java FLOAT/DOUBLE + TS_2DIFF layout is the canonical layout for cross-language compatibility. The C++ writer and reader use this format as their compatibility boundary.
• The compatibility scope does not include the raw bit-cast layout produced by the earlier C++ FLOAT/DOUBLE + TS_2DIFF writer.

# C++ layout introduced by #796
[overflow marker 1][blockValueCount 1][bitmap(s) 1][maxPointNumber][block 1]
[overflow marker 2][blockValueCount 2][bitmap(s) 2][maxPointNumber][block 2]
[overflow marker 3][blockValueCount 3][bitmap(s) 3][maxPointNumber][block 3]

# C++ layout in the current PR
[overflow marker 1][blockValueCount 1][bitmap(s) 1][maxPointNumber][block 1]
[overflow marker 2][blockValueCount 2][bitmap(s) 2]                [block 2]
[overflow marker 3][blockValueCount 3][bitmap(s) 3]                [block 3]

# Canonical Java layout
[overflow marker][pageValueCount][page-wide bitmap(s)]
[maxPointNumber][block 1][block 2][block 3]

I reviewed the Java and C++ encoder/decoder implementations and their history again. In the earlier discussion, I mixed together the official Java FLOAT/DOUBLE format, the raw format produced by the early C++ writer, and the per-block prefix format produced by the later C++ writer. I apologize for the confusion. The sections below describe these formats separately and note the remaining boundaries in the current code.

1. How Java Handles FLOAT/DOUBLE TS_2DIFF

TS_2DIFF itself encodes integers. Java adds a FLOAT/DOUBLE wrapper around it:

FLOAT  -> int32 -> IntDeltaEncoder
DOUBLE -> int64 -> LongDeltaEncoder

Suppose maxPointNumber = 2, so maxPointValue = 100. Java handles each value in one of three ways:

Condition	Stored representation	Decoding
value * 100 fits in the target integer type	round(value * 100)	Divide by 100
The scaled value overflows, but the original value fits in the target integer type	round(value)	Divide by 1
The original value cannot be converted safely, or it is NaN/Infinity	Result of floatToIntBits / doubleToLongBits	Restore from the raw bits

To distinguish these cases, Java writes one or two page-wide bitmaps when needed.

One detail is that Java uses Float.floatToIntBits / Double.doubleToLongBits. Regular values and Infinity retain their corresponding IEEE 754 bit patterns, while NaN is converted to Java's canonical NaN bit pattern.

The Java page layout has three forms:

# Every value can be scaled normally
[maxPointNumber]
[TS_2DIFF block 1][TS_2DIFF block 2]...

# At least one scaled value overflows, but no original value overflows
[Integer.MAX_VALUE]
[pageValueCount]
[page-wide scaled-value bitmap]
[maxPointNumber]
[TS_2DIFF block 1][TS_2DIFF block 2]...

# At least one value is stored as its raw IEEE 754 bits
[Integer.MAX_VALUE - 1]
[pageValueCount]
[page-wide scaled-value bitmap]
[page-wide original-value bitmap]
[maxPointNumber]
[TS_2DIFF block 1][TS_2DIFF block 2]...

The key points are:

• maxPointNumber appears once per page.
• The overflow flags and bitmaps are also page-level metadata.
• Each bitmap covers the entire page rather than one TS_2DIFF block.
• The metadata is followed by a continuous sequence of integer TS_2DIFF blocks, with no additional FLOAT/DOUBLE prefix between blocks.

2. The Previously Mentioned Raw TS_2DIFF Path

Before #796, the C++ FloatTS2DIFFEncoder / DoubleTS2DIFFEncoder interpreted each floating-point value as an integer of the same width and then applied integer TS_2DIFF:

float IEEE bits -> int32 -> integer TS_2DIFF -> int32 -> float IEEE bits

double IEEE bits -> int64 -> integer TS_2DIFF -> int64 -> double IEEE bits

This approach preserves the floating-point bits losslessly. It is not the Java FLOAT/DOUBLE TS_2DIFF format, but it was once the official C++ writer output when FLOAT/DOUBLE + TS_2DIFF was selected explicitly.

My earlier regression test used IntTS2DIFFEncoder / LongTS2DIFFEncoder to encode the same IEEE bit patterns, producing a payload equivalent to this earlier writer path. The test demonstrated that the new prefix detection could misclassify a historical raw block as a Java floating-point prefix and eventually cause the decoder to hang.

The current writer no longer produces the raw layout, while the reader's handling of it represents compatibility logic for historical C++ files.

The raw layout was private to the early C++ implementation and was never supported by the Java reader, so it is not part of the cross-language TsFile format. Detecting the raw and Java layouts from the input bytes retains this historical behavior in the decoder state machine and also introduces format ambiguity.

From the perspective of format boundaries and implementation complexity, I prefer focusing the C++ decoder on the canonical Java layout and returning a format error for the earlier raw layout. This simplifies prefix handling and avoids continuing to decode after a misclassification. If the community wants to retain support for early C++ files, that behavior can be discussed separately with a more explicit format identifier.

3. C++ Writer Layout After the Java-Style Wrapper Was Introduced

#796 changed C++ FLOAT/DOUBLE TS_2DIFF from raw bit-casting to Java-style scaling, maxPointNumber, and overflow bitmaps, followed by integer TS_2DIFF.

However, the integer encoder triggers a block flush after accumulating 129 values, and FloatTS2DIFFEncoder::flush() also writes the floating-point wrapper metadata.

As a result, this version of the C++ writer produces a per-block layout. For a page containing three blocks where every block has an overflow value, the layout is:

[overflow marker 1][blockValueCount 1][bitmap(s) 1][maxPointNumber][block 1]
[overflow marker 2][blockValueCount 2][bitmap(s) 2][maxPointNumber][block 2]
[overflow marker 3][blockValueCount 3][bitmap(s) 3][maxPointNumber][block 3]

A block without overflow uses:

[maxPointNumber][block]

This layout results from the integer block flush and the FLOAT/DOUBLE wrapper flush sharing the same flush() method. After every 129 values, the integer encoder invokes the virtual flush() method. During that call, the floating-point encoder generates the bitmap for the current block, writes it, and clears underflow_flags_. The next block collects a new set of flags and generates another bitmap.

The difference from the Java page-wide layout is the metadata scope. This discussion uses the Java layout as the format baseline: the target C++ encoder/decoder layout has page-wide metadata, while the earlier C++ per-block layout is outside the compatibility scope.

The default encoding for FLOAT/DOUBLE is GORILLA. An explicit TS_2DIFF schema choice still selects this C++ writer path.

4. Format Differences That Remain in the Current PR

This PR uses max_point_number_saved_ to avoid writing maxPointNumber repeatedly for every block. A multi-block page without overflow now has this layout:

[maxPointNumber][block 1][block 2]...

This part matches Java.

One remaining difference concerns metadata scope: underflow_flags_ is still cleared after each block flush, and each overflow bitmap is still generated per block. When overflow occurs, C++ therefore continues to write per-block metadata rather than Java's page-wide metadata.

The expanded comparison of the three layouts appears in the TL;DR.

The current PR removes the repeated maxPointNumber before later blocks, but the bitmaps remain per-block, so the resulting layout still differs from Java.

The decoder follows the same per-block model. When entering a later block, it clears the bitmap and resets segment_pos_ to 0. When reading a multi-block overflow page produced by Java, it loses the page-wide bitmap state after the first block.

Another related boundary is maxPointNumber = 0. The Java encoder supports this configuration, for which the first byte of a canonical page is 0x00. The current prefix detection treats a leading 0x00 byte as a legacy raw payload. For a simple payload whose first value is 1.0, the C++ decoder returns E_OK but produces approximately 2.35099e-38.

For canonical Java format compatibility, the current PR has completed the change that writes maxPointNumber once per page. The overflow metadata and decoder bitmap state remain block-scoped.

5. One Possible Implementation Direction

The encoder could collect floating-point conversion state across the entire page and generate the flags, bitmaps, and maxPointNumber once during the page flush, followed by a continuous sequence of integer TS_2DIFF blocks.

The decoder could parse the FLOAT/DOUBLE metadata once at the beginning of the page and retain the bitmap and current value position throughout the page. When it enters a new integer block, it would continue using the same page-wide bitmap and position.

The C++ implementation uses the canonical Java layout as its format baseline. The automatic detection and fallback logic that distinguishes the raw layout, the earlier C++ per-block layout, and the Java layout can be simplified at the same time. The decoder then maintains only the Java format state machine, while other inputs return a format error.

The existing batch decoder remains reusable:

1. Parse the page-level FLOAT/DOUBLE metadata once
2. Decode the integer TS_2DIFF blocks with read_batch_int32/read_batch_int64
3. Look up the bitmap using the page-wide position
4. Convert the integers in the batch to FLOAT/DOUBLE

Per-value bitmap checks and numeric conversion remain, while the main bit unpacking, delta reconstruction, and SIMD batch paths can still be reused. FLOAT/DOUBLE batch reads can also reuse the main flow of the integer batch decoder.

6. Additional Validation for the Current PR

The existing Java/C++ compatibility test can be reused. Relevant locations include:

• Java: java/tsfile/src/test/java/org/apache/tsfile/compatibility/TableModelEncodingCompressionCompatibilityTest.java
• C++: cpp/test/reader/table_view/table_model_encoding_compression_compatibility_test.cc
• Workflow: .github/workflows/compatibility-test.yml

At present, Java's buildMatrix() and C++'s BuildMatrix() cover only CHIMP, RLBE, and CAMEL. They do not include FLOAT/DOUBLE + TS_2DIFF. Both sides also write only 32 points by default, so the tests do not cross the boundary of a TS_2DIFF block containing 129 values.

Two additions can extend this coverage:

encoding matrix += FLOAT + TS_2DIFF
encoding matrix += DOUBLE + TS_2DIFF
rowCount = 300  # apply to all compatibility cases

In other words, the matrices on both sides can include FLOAT/DOUBLE + TS_2DIFF, while the number of written points for every existing compatibility case can be increased to 300. These cases can continue using the existing fixture generation, manifest, and bidirectional validation flow.

One detail is worth noting: the current compatibility test uses exact-bit comparisons for FLOAT/DOUBLE, while TS_2DIFF applies a fixed-point conversion based on maxPointNumber. Values such as -0.0 and pi in the existing data may not retain their original bits after TS_2DIFF encoding. The new combinations can either use data that is restored exactly after scaling or reflect the TS_2DIFF conversion rules in their expected values.

To cover the page-wide bitmap across blocks, the compatibility cases can use rowCount = 300 and include scaled-overflow data among those 300 values. This crosses the 129-value block boundary and exercises both Java page-wide bitmap reads and C++ multi-block writer output.

A Java fixture with maxPointNumber = 0 can also cover the prefix boundary. It verifies that a canonical page beginning with 0x00 enters the correct parsing path.

I plan to cover a more complete cross-language compatibility matrix in a separate follow-up PR, including parameterized row counts, data types, encodings, and compression combinations. If increasing the row count reveals compatibility issues in other combinations, those can be tracked in separate issues.

[kkzi]

Hi @ColinLeeo, thanks for the detailed 8/24 clarification — the format baseline and the section-by-section layout analysis made the rework straightforward to scope. The branch is rebased onto develop (the compatibility infrastructure from #905 is now available) and implements the direction you outlined.

What changed

Encoder (page-wide metadata). The integer encoder's automatic 129-value block flush now emits a plain integer block into an internal page buffer; overflow flags and buffered blocks survive the block boundary. The page-seal flush emits the page metadata once — [overflow marker][pageValueCount][page-wide bitmap(s)][maxPointNumber] followed by the continuous block sequence — matching the canonical Java layout in all three forms. The per-block wrapper metadata is gone.

Bit width. While validating against the Java hex golden tests I found the old width computation (max - min) also wraps negative when raw deltas wrap the signed type (e.g. adjacent raw IEEE bit patterns), silently writing bit width 0 and discarding every delta. The encoder now widens each rebased delta individually, mirroring calculateBitWidthsForDeltaBlockBuffer.

Decoder (single grammar, page-wide state). All layout sniffing is removed (scan/looks_like_ts2diff_header/is_legacy_raw_/per-segment maxPointNumber handling). Page metadata is parsed exactly once per page; the bitmaps and the page position survive block transitions, so Java multi-block overflow pages decode correctly. maxPointNumber = 0 (page starting with 0x00) is a valid Form 1 page. The raw bit-cast layout and the pre-#910 per-segment layout now fail fast with E_TSFILE_CORRUPTED: block headers are validated (write_index ∈ [0,128], width in range) and a truncated header fails instead of reusing stale state, which was the root of the end-of-input spin — batch readers can no longer loop on out-of-format input. The legacy regression tests were rewritten to assert fail-fast behavior.

Batch reads. FLOAT/DOUBLE read_batch reuses the integer batch decoder (SIMD fast path) and applies the page-wide bitmaps afterwards per page position, as you sketched in §5.

Tests. Both matrices now include FLOAT + TS_2DIFF and DOUBLE + TS_2DIFF and every case writes 300 rows (129+129+42). The value set covers scaled, scale-overflow (1.5e9f / 1e18, reachable only at mpn > 0), and raw-bit forms (canonical NaN, ±Inf). Every chosen value restores identically at maxPointNumber 0 and 2, so the validating reader never needs to know which writer produced a file; expectations are computed from the encoding rules rather than the input bits. A wire-format contract document derived from the Java reference implementations is included (cpp/docs/ts2diff-float-double-wire-format.md).

Verification

All four compatibility directions on the extended matrix pass bit-exactly (30 cases each, LZMA2 excluded locally — see below), including FLOAT/DOUBLE TS_2DIFF across the block boundary with page-wide bitmaps. Full C++ suite: 787 tests, 784 pass / 3 skipped (env-gated compat fixtures). Java reads Java's 45 fixtures (LZMA2 included) cleanly.

Two things to flag

1. LZMA2 on Windows/MSVC Debug: write_table fails with E_INVALID_ARG for any encoding when LZMA2 is selected (reproducible with CHIMP + LZMA2 on develop's feat(cpp): refactor dependency sourcing and extend codec compatibility #905 without my changes), so the 15 LZMA2 cases fail locally on the C++ side. Java handles LZMA2 fine, which suggests a C++-side (possibly MSVC-specific) issue in feat(cpp): refactor dependency sourcing and extend codec compatibility #905. Happy to file a separate issue with the repro if useful.
2. C++ default max_point_number_ = 2 vs Java's Ts2Diff builder hard-coded 0: the value is self-describing in the stream so interop is unaffected, but since "Java is the format baseline" I can align the default to 0 in this PR or a follow-up, whichever you prefer.

The commits are structured as: matrix + contract doc, encoder rework, decoder rework. Glad to restructure or address anything else.

[kkzi]

Follow-up on point 2: ff7bfee aligns the C++ FloatTS2DIFFEncoder/DoubleTS2DIFFEncoder default max_point_number to the Java Ts2Diff builder value (0), so pages written by either implementation are now byte-identical by default.

Notes from the change:

• The scale-overflow form (Form 2) is unreachable at mpn = 0 — the scaled product equals the value itself, so any overflow is a value overflow taking the raw-bits path. The wire-format doc records this; the compatibility value set keeps the former Form-2 entries (1.5e9f / 1e18), which now exercise the raw-bits path, and expectations are unchanged (the set is mpn-agnostic by construction).
• The Java hex goldens were regenerated with mpn = 0.
• Tests that counted 0x02 prefix bytes were replaced with a structural page walker (parse the metadata once, then verify a continuous well-formed block stream) — byte counting cannot distinguish the 0x00 mpn byte from block-header high bytes.
• Round-trip ramp data was integerized so expectations hold under the default mpv = 1.

Re-verified: full C++ suite 784/787 pass, and all four compatibility directions remain green on the 30 non-LZMA2 cases.

[kkzi]

Pushed c23e205: the previous 0fca3e4 had the <fcntl.h> include block placed after the extensionless C++ headers in utf8_file_open.h — that ordering matches local clang-format 22, but not the pinned 17.0.6 that spotless uses in CI, so all unit-test jobs failed the spotless-check goal before any test ran. The include block now follows the 17.0.6 IncludeBlocks: Regroup order, and I verified the whole cpp/src tree (323 files) with clang-format 17.0.6 --dry-run --Werror locally.

No functional changes — include ordering only (amended into the same style commit, with the message corrected to the actual ordering).

The CI runs for this push are again waiting for approval (action_required) — could you approve the workflows so they can re-run? Thanks!

[ColinLeeo]

The final TS_2DIFF decoder is forward-only and no longer probes or rewinds the stream, so the ByteStream::check_space() change is no longer required by this fix. Recomputing read_page_ from head_ at every page boundary also changes sequential traversal from O(n) to O(n²). Please revert this change.

[kkzi]

Reverted in f1f4e47 — check_space() is back to the next_ advance and byte_stream.h is now byte-identical to develop. The codec-test hex helper also no longer rewinds via set_read_pos (it reads from position 0 directly), so nothing in this PR depends on the boundary-parked-cursor behavior anymore.

[ColinLeeo]

TSEncodingBuilder.Ts2Diff initializes maxPointNumber to 0, but initFromProps() replaces it with the schema’s max_point_number value, or with TSFileConfig.floatPrecision when the property is absent. The standard schema writer path invokes initFromProps(); its current default is therefore 2.

[kkzi]

You are right — fixed in f1f4e47. The Encoder Construction section now describes the actual chain: the Ts2Diff field initializes 0, but MeasurementSchema.getValueEncoder() always calls initFromProps(), which substitutes the schema max_point_number or TSFileConfig.floatPrecision (current default 2) when the property is absent. The doc no longer claims Java hard-codes 0.

[ColinLeeo]

My suggestion was to add an explicit Java fixture with maxPointNumber = 0 to cover the valid 0x00 page-prefix boundary.
It was not a request to change the C++ writer default. The standard Java schema writer initializes Ts2Diff from properties and falls back to TSFileConfig.floatPrecision, whose current default is 2. Please restore the C++ default to 2 and keep mpn = 0 as an explicit compatibility test case.

[kkzi]

Done as suggested in f1f4e47: the C++ default is restored to 2 (matching the standard initFromProps -> TSFileConfig.floatPrecision path), and mpn = 0 is now an explicit fixture rather than the default:

• C++ encoder gained set_max_point_number(); MaxPointNumberZeroPagePrefix builds a Form 1 page starting with 0x00 (140 rows crossing the block boundary) and round-trips both FLOAT and DOUBLE.
• The Java matrix generates *.mpn0 fixtures that pass max_point_number=0 props through MeasurementSchema, 6 new cases (FLOAT/DOUBLE × 3 compressions). Both readers validate them, so the canonical 0x00 page prefix is exercised cross-language.

One independent bug this fixture exposed: MeasurementSchema::deserialize_from never consumed the (key, value) props pairs (the loop iterated props_.size(), which is 0 at that point), so any Java schema carrying props desynced the TsFileMeta bloom filter into E_TSFILE_CORRUPTED. That pre-existing bug is fixed in the same commit — it was simply never reachable before because no fixture used props.

[ColinLeeo]

What is the format-level basis for limiting maxPointNumber to 100? Neither the Java Ts2Diff builder nor the wire-format document defines this bound, and Java can produce valid pages with values greater than 100. This check therefore rejects otherwise valid Java-format input. Please remove the arbitrary limit, or define and enforce a shared bound across the Java/C++ encoders and decoders with corresponding documentation and tests.

[kkzi]

Removed in f1f4e47 — both checks are gone. As you noted, neither the Java builder nor the wire format defines this bound. Java accepts any varint the stream carries; for very large mpn Math.pow overflows to +inf and every value takes the raw-bits path. MaxPointNumberAboveLegacyBoundDecodes now covers mpn = 1000 (all values stored/restored as raw bits, byte-exact).

[ColinLeeo]

128 is DeltaBinaryEncoder.BLOCK_DEFAULT_SIZE, not a serialized wire-format limit. The block header already carries writeIndex, and Java exposes constructors with a configurable block size.

Rejecting writeIndex > 128 therefore prevents C++ from reading otherwise valid Java TS_2DIFF blocks. Please validate writeIndex against the declared page value count and available packed bytes instead of the encoder’s default block size.

[kkzi]

Fixed in f1f4e47 as you suggested — writeIndex is now validated against availability, not the encoder default:

• read_header: write_index * bit_width (int64 arithmetic) must fit in the stream remainder; write_index itself has no upper bound beyond that. Documented in the wire-format doc (BLOCK_DEFAULT_SIZE is a buffer size, not a wire limit).
• The skip paths got the same bound, plus a grouped header-read check — truncated headers previously acted on stale stack values.
• LargeBlockBeyondDefaultSizeDecodes hand-builds a 200-value block (wi = 199) and decodes it through both the scalar and batch paths.

Re-auditing this change surfaced one gap it had opened: the old <= 128 check had been masking the absence of the read_long end-of-input bailout (dropped in 67ab361), so a truncated page whose header passes the availability check could spin the scalar path forever — reproduced with a 17-byte page under a watchdog. The bailout is restored and pinned by ScalarReadTerminatesOnTruncatedBlock. Two related fixes rode along: peek_next_block_range_int64 returned E_TSFILE_CORRUPTED from a bool function (converted to true, callers acted on a stale range) — now returns false; and skip_peeked_block_int64 widened its byte computation to int64.

[kkzi]

Hi @ColinLeeo, thanks for the detailed review — all five comments are addressed in f1f4e47 (replied inline).

Summary:

1. check_space() reverted; byte_stream.h is byte-identical to develop again.
2. Doc corrected: the standard Java schema path is initFromProps → TSFileConfig.floatPrecision (default 2), not a hard-coded 0.
3. C++ default restored to 2; mpn = 0 is now an explicit fixture — a set_max_point_number() test path plus six Java *.mpn0 fixtures carrying max_point_number=0 props, validated by both readers (canonical 0x00 page prefix covered cross-language). The fixture exposed a pre-existing MeasurementSchema::deserialize_from bug (props pairs never consumed → TsFileMeta bloom filter desync → E_TSFILE_CORRUPTED); fixed in the same commit.
4. mpn > 100 bound removed; covered by an mpn = 1000 test.
5. writeIndex validated against availability (int64 arithmetic) instead of BLOCK_DEFAULT_SIZE; skip paths hardened with the same bound plus a grouped header-read check; a 200-value block test covers scalar and batch paths.

Re-auditing the relaxed bound surfaced that the old <= 128 check had been masking a missing read_long end-of-input bailout (dropped in 67ab361) — a truncated page could spin the scalar decoder forever (reproduced with a 17-byte page). Restored and pinned by a regression test. Also fixed peek_next_block_range_int64 returning an error code from a bool function, and rounded the conversions to Java Math.round semantics (floor(x + 0.5), ties towards +infinity, saturating at the integer limits) so negative half-way values now store byte-identically to the Java writer.

Verified: full C++ suite 803 passed / 3 skipped (env-gated compat); compatibility matrix green in all four directions including the new mpn0 fixtures; clang-format 17.0.6 clean. LZMA2 cases still excluded locally due to the separate Windows/MSVC issue that reproduces on develop.

The CI runs for the new push are waiting for approval (action_required) — could you approve the workflows so they can re-run? Thanks!

[kkzi]

Force-pushed 7241ca6. Two things changed since the 8/28 review reply: the PR scope was narrowed, and two more decoder defects were fixed. Details below, plus corrections to two claims I made earlier.

Scope narrowed to TS_2DIFF only

The branch previously carried two unrelated blocks that I have removed and rebased out of the history entirely (not reverted on top, so the log no longer shows an add-then-remove round trip):

• Windows UTF-8 paths (open_utf8() via _wopen, plus the read_file / write_file / restorable-writer call sites)
• TsFileReader::get_timeseries_schema reporting the file's real encoding/compression instead of library defaults

Both are real fixes and I will open separate PRs for them, with their own reproductions and tests. Neither belongs in a TS_2DIFF wire-format change. The cumulative diff here is now 7 files: the encoder, decoder, MeasurementSchema::deserialize_from, the codec tests, both compat tests, and the wire-format doc.

Worth noting for review: dropping the reader change does not weaken the compat generator's on-wire codec assertion. AssertOnWireCodec opens its own ReadFile, seeks to offset_of_chunk_header_ and deserializes the ChunkHeader directly — it never calls get_timeseries_schema. I verified this by running the generator on the rebased branch; the assertion still holds for every case.

Two further decoder fixes (7241ca6)

Re-auditing the relaxed writeIndex bound from review comment 5 turned up two problems in the code I had just written:

1. The page-value-count bound was defeated by int32 overflow on exactly the input it guards. read_header compared write_index + 1 > max_values_ in int32; at write_index == INT32_MAX that wraps negative and passes. A page with writeIndex = INT32_MAX, bitWidth = 0 therefore slipped through — a zero-width block occupies no packed bytes, so the availability bound cannot catch it either. Now computed in int64. RejectsBlockBeyondPageValueCount builds that page by hand and pins E_TSFILE_CORRUPTED.

2. peek_next_block_range_int64 still computed packed_bytes in int32 — the one path I had missed when widening the others, and the value is used as a raw-pointer offset, so a wrapped negative result reads outside the buffer. Widened to match read_header and skip_peeked_block_int64.

The same commit finishes the error propagation: read_int32 / read_int64 / read_float / read_double returned E_OK unconditionally and the delta_min / first_value reads discarded the return of read_i32 / read_i64, so a truncated page surfaced as plausible-looking values rather than an error. Failures now latch and propagate out of the read_* entry points, covered by ScalarReadRejectsTruncatedFixedFields.

LegacyPerSegmentMaxPNRejected was rewritten as a consequence: its old assertion counted value mismatches, which only worked while the decoder returned poison values instead of an error. It now asserts that segment 1 still decodes and that the out-of-format continuation stops before producing the expected sequence.

Two corrections to the PR body

Both were claims I had not checked closely enough; the body is now updated.

• set_max_point_number() has no production caller. encoder_factory.h constructs FloatTS2DIFFEncoder / DoubleTS2DIFFEncoder with no props, so the C++ write path cannot currently request a non-default precision. The setter exists so the layout is encodable and is exercised only by the tests that pin mpn = 0 and mpn = 1000. The default remains 2, matching initFromProps → TSFileConfig.floatPrecision.
• The *.mpn0 fixtures are Java-generated and C++-validated only, not bidirectional as I implied. C++ BuildMatrix() does not emit mpn0 cases, which follows from the point above.

Verification on the rebased branch

• Full C++ suite: 805 passed / 3 skipped of 808 (the 3 skips are env-gated by design: two compat entry points and the external dataset index).
• Compat matrix re-run in all four directions on the 30 non-LZMA2 cases: cpp→cpp, cpp→java, java→cpp (includes the 4 non-LZMA2 mpn0 fixtures), java→java. All pass.
• The rebase changed history only, not content: the tree hash of the rebased tip is identical (fb4b8b9) to the pre-rebase tip.
• clang-format 17.0.6 clean on all changed files.
• The 15 LZMA2 generate cases still fail locally with E_INVALID_ARG from write_table for any encoding — the separate Windows/MSVC issue that reproduces on develop without these changes. Left to CI.

The new push is again waiting for workflow approval (action_required) — could you approve it? Thanks.