You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@tsfile.apache.org by "kkzi (via GitHub)" <gi...@apache.org> on 2026/08/07 09:28:08 UTC
[PR] fix(cpp): handle TS2DIFF float prefixes in batch decode [tsfile]
kkzi opened a new pull request, #901:
URL: https://github.com/apache/tsfile/pull/901
## Summary
- make C++ FLOAT/DOUBLE TS_2DIFF batch decoding consume the Java-compatible scale/overflow prefix
- route batch reads through the existing segment-aware scalar decoders
- add regression coverage for multiple FLOAT segments and a DOUBLE overflow bitmap prefix
## 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 `maxPointNumber` or 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_float` and `read_double` implementations 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_PartialPaths`
- `TsFileWriterTest.WriteDiffrentTypeCombination`
- `clang-format --dry-run --Werror cpp/src/encoding/ts2diff_decoder.h cpp/test/encoding/ts2diff_codec_test.cc`
Fixes #900
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5487159840
Pushed `1d0a704` with the remaining fixes. Full suite passes locally (805 tests, MSVC).
- **Bitmap guard** (`read_page_meta`): now `bm_len * (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ? 2 : 1) > remaining_size()` as suggested — the previous comparison had the multiplication on the wrong side, so a two-bitmap page did not require the bytes it should.
- **`header_error_`**: deleted; `read_error_` is the single sticky-error flag (latched by header/fixed-field read failures, surfaced through the value-returning `decode()`).
- **`cal_bit_width()`**: deleted; only `cal_bit_width_rebased()` remains.
- **Error codes**: format violations (bad bit width, oversized bitmap, length mismatch, out-of-range index) now return `E_DECODE_ERR`, and short reads propagate the underlying stream error (`E_PARTIAL_READ`) instead of being rebranded as `E_TSFILE_CORRUPTED` — the same convention the RLE decoder already uses. Tests updated accordingly.
- **Comments**: process/iteration-trace comments removed across the diff; the remaining ones describe current behavior only.
- Also converged the duplicated `FLAG_*` overflow-marker definitions in the decoder/encoder headers into one shared header, `ts2diff_wire_format.h`.
On the bounds discussion: the final code applies no arbitrary limits — `maxPointNumber` is unbounded (Java accepts whatever the varint carries; the compat fixtures cover `mpn = 0` and `mpn = 1000`), and `writeIndex` is bounded only by the bytes the stream can actually supply.
One finding deliberately left out of scope: a **form-1 page** (which carries no page-value-count metadata) with a crafted `bit_width = 0` / `writeIndex = INT32_MAX` block can drive the decoder to produce up to 2^31 values from a few bytes, because zero-width blocks consume no packed bytes and the availability bound cannot constrain `writeIndex`. This path is pre-existing on `develop` (form 1 is not introduced by this PR), so I will address it in a separate issue/PR rather than expanding this one. For reference, Java does not accept the equivalent input either — `DeltaBinaryDecoder.loadIntBatch` allocates `new int[packNum]`, which throws for `packNum = INT32_MAX`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3877212755
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -328,9 +322,30 @@ class TS2DIFFDecoder : public Decoder {
current_index_ != 0);
}
- void read_header(common::ByteStream& in) {
- common::SerializationUtil::read_i32(write_index_, in);
- common::SerializationUtil::read_i32(bit_width_, in);
+ // Reads the 4+4 byte block header. On a truncated stream the old
+ // signature silently kept the previous (stale) write_index_, letting a
+ // caller loop forever re-emitting a phantom block; the failure is
+ // recorded in header_error_ (decode() returns a value, not an error
+ // code) and returned for batch entry points.
+ int read_header(common::ByteStream& in) {
+ int32_t write_index = 0;
+ int32_t bit_width = 0;
+ if (common::SerializationUtil::read_i32(write_index, in) !=
+ common::E_OK ||
+ common::SerializationUtil::read_i32(bit_width, in) !=
+ common::E_OK) {
+ header_error_ = true;
+ return common::E_TSFILE_CORRUPTED;
+ }
+ if (write_index < 0 || write_index > 128 || bit_width < 0 ||
+ bit_width > (int)sizeof(T) * 8) {
+ header_error_ = true;
+ return common::E_TSFILE_CORRUPTED;
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5392099508
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3877208124
##########
cpp/src/common/allocator/byte_stream.h:
##########
@@ -696,7 +696,18 @@ class ByteStream {
if (UNLIKELY(read_page_ == nullptr)) {
read_page_ = head_.load();
} else if (UNLIKELY((read_pos_ & page_mask_) == 0)) {
- read_page_ = read_page_->next_.load();
+ // At a page boundary the cursor may have been parked here by a
+ // preceding sequential read (read_page_ is the page just
+ // finished, advance one) or by set_read_pos() (read_page_ is
+ // already the boundary page, advancing would skip it). The
+ // two states are indistinguishable, so recompute the page
+ // from the head instead of blindly following next_.
+ Page* p = head_.load();
+ uint64_t page_idx = read_pos_ / page_size_;
+ while (p != nullptr && page_idx-- > 0) {
+ p = p->next_.load();
+ }
+ read_page_ = p;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3877209423
##########
cpp/docs/ts2diff-float-double-wire-format.md:
##########
@@ -0,0 +1,138 @@
+<!-- Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License. -->
+
+# FLOAT/DOUBLE TS_2DIFF Wire Format (Java Canonical Layout)
+
+This document specifies the canonical on-disk layout of FLOAT/DOUBLE TS_2DIFF
+pages, derived from the Java reference implementation
+(`FloatEncoder`, `FloatDecoder`, `DeltaBinaryEncoder`, `BitMap`).
+The Java layout is the cross-language compatibility boundary. Other layouts
+produced by earlier C++ writers (raw bit-cast, per-block wrapper metadata) are
+implementation artifacts outside the compatibility scope; the C++ decoder
+treats them as a format error.
+
+## Encoding Pipeline
+
+TS_2DIFF encodes integers. Floating-point values go through a wrapper that
+converts each value to an integer, encodes the integers with
+`IntDeltaEncoder` (FLOAT) or `LongDeltaEncoder` (DOUBLE), and emits page-wide
+conversion metadata.
+
+Given `maxPointNumber = mpn` and `maxPointValue = 10^mpn` (`mpn <= 0` implies
+`maxPointValue = 1`), each value maps to one of three stored forms:
+
+| Condition | Stored bits | Decoder action |
+| -------------------------------------- | --------------------------- | ------------------------- |
+| `round(v * 10^mpn)` fits the int type | `round(v * 10^mpn)` | divide by `10^mpn` |
+| scaled overflows but `v` itself fits | `round(v)` | divide by `1` |
+| `v` out of int range, or NaN | `floatToIntBits(v)` / `doubleToLongBits(v)` | restore raw bits |
+
+The three forms are tracked per page as a tri-state flag list
+(`underflowFlags` in Java):
+
+- `true` -> scaled form
+- `false` -> rounded form (scale overflow)
+- `null` -> raw IEEE 754 bits (value overflow or NaN)
+
+## Page Layout
+
+```text
+# Form 1: every value stored in scaled form (no bitmap at all)
+[maxPointNumber varint]
+[TS_2DIFF block 1][TS_2DIFF block 2]...[final block]
+
+# Form 2: at least one value is 'false' (scale overflow), none is 'null'
+[Integer.MAX_VALUE varint] # 0xFF 0xFF 0xFF 0xFF 0x07
+[pageValueCount varint]
+[scaled-bitmap, pageValueCount/8+1 bytes] # marks 'true' entries
+[maxPointNumber varint]
+[TS_2DIFF block 1]...[final block]
+
+# Form 3: at least one value is 'null' (raw bits)
+[Integer.MAX_VALUE-1 varint] # 0xFF 0xFF 0xFF 0xFF 0x06
+[pageValueCount varint]
+[scaled-bitmap, pageValueCount/8+1 bytes] # marks 'true' entries
+[raw-bitmap, pageValueCount/8+1 bytes] # marks 'null' entries
+[maxPointNumber varint]
+[TS_2DIFF block 1]...[final block]
+```
+
+Key invariants:
+
+- `maxPointNumber` appears exactly once per page, before the first integer
+ block (for Forms 2/3 it appears after the bitmaps).
+- The bitmaps cover the entire page, not individual TS_2DIFF blocks. The
+ decoder keeps one page-wide `position` that never resets between blocks;
+ only a page-level `reset()` clears it.
+- Bitmap byte length is always `size/8 + 1`, even when `size % 8 == 0`
+ (`BitMap.getSizeOfBytes`).
+- Bitmap bit order is LSB-first within each byte: position `p` maps to
+ `bits[p / 8] & (1 << (p % 8))`.
+- `pageValueCount` counts all values of the page (across blocks).
+- A first-page byte of `0x00` is the normal encoding of `maxPointNumber = 0`,
+ which is the Java `Ts2Diff` builder default. It is not a legacy marker.
+
+## Integer Block Layout
+
+Identical to the integer TS_2DIFF format (`DeltaBinaryEncoder`):
+
+```text
+[writeIndex int32 BE] # number of values in this block (<= 129)
+[bitWidth int32 BE]
+[block-specific header] # first value; min delta
+[packed data] # writeIndex * bitWidth bits
+```
+
+`BLOCK_DEFAULT_SIZE = 128`: the encoder buffers the first value plus up to 128
+deltas, then flushes a 129-value block. A 300-value page therefore produces
+blocks of 129, 129, 42.
+
+## Encoder Construction
+
+Java `TSEncodingBuilder.Ts2Diff` hard-codes `maxPointNumber = 0` for
+FLOAT/DOUBLE (it does not read `max_point_number` props). Pages produced by
+Java therefore start with `0x00`, and the C++ `FloatTS2DIFFEncoder` /
+`DoubleTS2DIFFEncoder` use the same default. The value stored in the
+stream is self-describing, so files written by other `maxPointNumber`
+values remain readable.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5450839389
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3877210509
##########
cpp/src/encoding/ts2diff_encoder.h:
##########
@@ -553,18 +588,22 @@ int TS2DIFFEncoder<T>::encode_batch(const int64_t* values, uint32_t count,
class FloatTS2DIFFEncoder : public TS2DIFFEncoder<int32_t> {
public:
- FloatTS2DIFFEncoder() : max_point_number_(2), max_point_value_(100.0) {}
+ FloatTS2DIFFEncoder()
+ : max_point_number_(0), // Java Ts2Diff builder default
+ max_point_value_(1.0),
+ page_blocks_(1024, common::MOD_TS2DIFF_OBJ, false) {}
int do_encode(float value, common::ByteStream& out_stream) {
Review Comment:
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.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,73 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
+ bool has_scaled_bm = false;
+ bool has_raw_bm = false;
+ std::vector<uint8_t> scaled_bm;
+ std::vector<uint8_t> raw_bm;
+ int max_point_number = 0;
+ int page_value_count = 0;
+};
-inline int consume_float_double_ts2diff_prefix(
- common::ByteStream& in, bool& is_legacy_raw, int& max_point_number,
- std::vector<uint8_t>& underflow_bm, std::vector<uint8_t>& overflow_bm,
- int& segment_size) {
+inline int read_page_meta(common::ByteStream& in, PageMeta& meta) {
int ret = common::E_OK;
- is_legacy_raw = false;
- max_point_number = 0;
- underflow_bm.clear();
- overflow_bm.clear();
- segment_size = 0;
- uint64_t mark = in.read_pos();
uint32_t tag = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(tag, in))) {
return ret;
}
- if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ||
- tag == FLAG_SCALED_VALUE_OVERFLOW) {
- uint32_t n = 0;
- if (RET_FAIL(common::SerializationUtil::read_var_uint(n, in))) {
+ if (tag == FLAG_SCALED_VALUE_OVERFLOW ||
+ tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
+ uint32_t count = 0;
+ if (RET_FAIL(common::SerializationUtil::read_var_uint(count, in))) {
return ret;
}
- segment_size = static_cast<int>(n);
- int bm_len = segment_size / 8 + 1;
- underflow_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (count == 0 || count > 0x7FFFFFFFu) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ const int bm_len = static_cast<int>(count) / 8 + 1;
+ meta.has_scaled_bm = true;
+ meta.scaled_bm.resize(static_cast<size_t>(bm_len), 0);
uint32_t read_len = 0;
- if (RET_FAIL(in.read_buf(underflow_bm.data(),
+ if (RET_FAIL(in.read_buf(meta.scaled_bm.data(),
static_cast<uint32_t>(bm_len), read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
- overflow_bm.resize(static_cast<size_t>(bm_len), 0);
- if (RET_FAIL(in.read_buf(overflow_bm.data(),
+ meta.has_raw_bm = true;
+ meta.raw_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (RET_FAIL(in.read_buf(meta.raw_bm.data(),
static_cast<uint32_t>(bm_len),
read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
}
+ meta.page_value_count = static_cast<int>(count);
uint32_t mpn = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(mpn, in))) {
return ret;
}
- max_point_number = static_cast<int>(mpn);
- return common::E_OK;
- }
-
- // Distinguish Java maxPointNumber prefix from legacy raw C++ block.
- max_point_number = static_cast<int>(tag);
- if (!looks_like_ts2diff_header(in)) {
- in.set_read_pos(mark);
- is_legacy_raw = true;
+ if (mpn > 100) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ meta.max_point_number = static_cast<int>(mpn);
} else {
- segment_size = 0;
+ if (tag > 100) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ meta.max_point_number = static_cast<int>(tag);
+ meta.page_value_count = 0; // unknown until the blocks are decoded
}
return common::E_OK;
Review Comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5434970460
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5421066848
placeholder
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5394021480
# 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.
```text
# 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:
```text
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:
```text
# Every value can be scaled normally
[maxPointNumber]
[TS_2DIFF block 1][TS_2DIFF block 2]...
```
```text
# 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]...
```
```text
# 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:
```text
float IEEE bits -> int32 -> integer TS_2DIFF -> int32 -> float IEEE bits
```
```text
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:
```text
[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:
```text
[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:
```text
[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:
```text
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:
```text
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3891226640
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,78 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
Review Comment:
There should be a errorcode associated with encoding or decoding.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -328,11 +329,47 @@ class TS2DIFFDecoder : public Decoder {
current_index_ != 0);
}
- void read_header(common::ByteStream& in) {
- common::SerializationUtil::read_i32(write_index_, in);
- common::SerializationUtil::read_i32(bit_width_, in);
+ // Reads the 4+4 byte block header. On a truncated stream the old
+ // signature silently kept the previous (stale) write_index_, letting a
+ // caller loop forever re-emitting a phantom block; the failure is
+ // recorded in header_error_ (decode() returns a value, not an error
+ // code) and returned for batch entry points.
+ // writeIndex is bounded by availability, not by the encoder's
+ // BLOCK_DEFAULT_SIZE: Java exposes block-size constructors, so blocks
+ // larger than 129 values are valid input (apache/tsfile#901 review).
+ int read_header(common::ByteStream& in) {
+ int32_t write_index = 0;
+ int32_t bit_width = 0;
+ if (common::SerializationUtil::read_i32(write_index, in) !=
+ common::E_OK ||
+ common::SerializationUtil::read_i32(bit_width, in) !=
+ common::E_OK) {
+ header_error_ = true;
+ read_error_ = common::E_TSFILE_CORRUPTED;
+ return common::E_TSFILE_CORRUPTED;
Review Comment:
use errorcode associated with encoding or decoding.
##########
cpp/src/encoding/ts2diff_encoder.h:
##########
@@ -331,10 +363,11 @@ inline int TS2DIFFEncoder<int64_t>::flush(common::ByteStream& out_stream) {
if (write_index_ == -1) {
return common::E_OK;
}
+ // Bit width over the raw deltas rebased by min; see the int32
+ // specialization for ordering rationale.
+ int bit_width = cal_bit_width_rebased();
// Subtract the minimum value for each delta_arr_ item
SIMDOps<int64_t>::rebase(delta_arr_, delta_arr_min_, write_index_);
- // Calculate the bit length of each value to writer
- int bit_width = cal_bit_width(delta_arr_max_ - delta_arr_min_);
// Header writes can fail too — see int32 specialization for rationale.
Review Comment:
delete the code of cal_bit_width() pls
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,78 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
+ bool has_scaled_bm = false;
+ bool has_raw_bm = false;
+ std::vector<uint8_t> scaled_bm;
+ std::vector<uint8_t> raw_bm;
+ int max_point_number = 0;
+ int page_value_count = 0;
+};
-inline int consume_float_double_ts2diff_prefix(
- common::ByteStream& in, bool& is_legacy_raw, int& max_point_number,
- std::vector<uint8_t>& underflow_bm, std::vector<uint8_t>& overflow_bm,
- int& segment_size) {
+inline int read_page_meta(common::ByteStream& in, PageMeta& meta) {
int ret = common::E_OK;
- is_legacy_raw = false;
- max_point_number = 0;
- underflow_bm.clear();
- overflow_bm.clear();
- segment_size = 0;
- uint64_t mark = in.read_pos();
uint32_t tag = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(tag, in))) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
- if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ||
- tag == FLAG_SCALED_VALUE_OVERFLOW) {
- uint32_t n = 0;
- if (RET_FAIL(common::SerializationUtil::read_var_uint(n, in))) {
- return ret;
+ if (tag == FLAG_SCALED_VALUE_OVERFLOW ||
+ tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
+ uint32_t count = 0;
+ if (RET_FAIL(common::SerializationUtil::read_var_uint(count, in))) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ if (count == 0 || count > 0x7FFFFFFFu) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ const int bm_len = static_cast<int>(count) / 8 + 1;
+ // Guard the allocation: a tiny corrupt page must not trigger a
+ // bitmap-sized allocation the stream cannot possibly back.
+ if (static_cast<uint64_t>(bm_len) >
+ (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ? 2 : 1) *
+ static_cast<uint64_t>(in.remaining_size())) {
+ return common::E_TSFILE_CORRUPTED;
}
Review Comment:
There should be:
if (static_cast<uint64_t>(bm_len) *
(tag == FLAG_ORIGINAL_VALUE_OVERFLOW ? 2 : 1) >
static_cast<uint64_t>(in.remaining_size())) {
??
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,78 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
+ bool has_scaled_bm = false;
+ bool has_raw_bm = false;
+ std::vector<uint8_t> scaled_bm;
+ std::vector<uint8_t> raw_bm;
+ int max_point_number = 0;
+ int page_value_count = 0;
+};
-inline int consume_float_double_ts2diff_prefix(
- common::ByteStream& in, bool& is_legacy_raw, int& max_point_number,
- std::vector<uint8_t>& underflow_bm, std::vector<uint8_t>& overflow_bm,
- int& segment_size) {
+inline int read_page_meta(common::ByteStream& in, PageMeta& meta) {
int ret = common::E_OK;
- is_legacy_raw = false;
- max_point_number = 0;
- underflow_bm.clear();
- overflow_bm.clear();
- segment_size = 0;
- uint64_t mark = in.read_pos();
uint32_t tag = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(tag, in))) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
- if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ||
- tag == FLAG_SCALED_VALUE_OVERFLOW) {
- uint32_t n = 0;
- if (RET_FAIL(common::SerializationUtil::read_var_uint(n, in))) {
- return ret;
+ if (tag == FLAG_SCALED_VALUE_OVERFLOW ||
+ tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
+ uint32_t count = 0;
+ if (RET_FAIL(common::SerializationUtil::read_var_uint(count, in))) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ if (count == 0 || count > 0x7FFFFFFFu) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ const int bm_len = static_cast<int>(count) / 8 + 1;
+ // Guard the allocation: a tiny corrupt page must not trigger a
+ // bitmap-sized allocation the stream cannot possibly back.
+ if (static_cast<uint64_t>(bm_len) >
+ (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ? 2 : 1) *
+ static_cast<uint64_t>(in.remaining_size())) {
+ return common::E_TSFILE_CORRUPTED;
}
- segment_size = static_cast<int>(n);
- int bm_len = segment_size / 8 + 1;
- underflow_bm.resize(static_cast<size_t>(bm_len), 0);
+ meta.has_scaled_bm = true;
+ meta.scaled_bm.resize(static_cast<size_t>(bm_len), 0);
uint32_t read_len = 0;
- if (RET_FAIL(in.read_buf(underflow_bm.data(),
+ if (RET_FAIL(in.read_buf(meta.scaled_bm.data(),
static_cast<uint32_t>(bm_len), read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
- overflow_bm.resize(static_cast<size_t>(bm_len), 0);
- if (RET_FAIL(in.read_buf(overflow_bm.data(),
+ meta.has_raw_bm = true;
+ meta.raw_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (RET_FAIL(in.read_buf(meta.raw_bm.data(),
static_cast<uint32_t>(bm_len),
read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
}
+ meta.page_value_count = static_cast<int>(count);
uint32_t mpn = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(mpn, in))) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
- max_point_number = static_cast<int>(mpn);
- return common::E_OK;
- }
-
- // Distinguish Java maxPointNumber prefix from legacy raw C++ block.
Review Comment:
When using agent tools, please handle intermediate/process information carefully. There is no need to introduce comments explaining something that does not exist in the final result. Please keep the implementation focused on the final outcome.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,78 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
+ bool has_scaled_bm = false;
+ bool has_raw_bm = false;
+ std::vector<uint8_t> scaled_bm;
+ std::vector<uint8_t> raw_bm;
+ int max_point_number = 0;
+ int page_value_count = 0;
+};
-inline int consume_float_double_ts2diff_prefix(
- common::ByteStream& in, bool& is_legacy_raw, int& max_point_number,
- std::vector<uint8_t>& underflow_bm, std::vector<uint8_t>& overflow_bm,
- int& segment_size) {
+inline int read_page_meta(common::ByteStream& in, PageMeta& meta) {
int ret = common::E_OK;
- is_legacy_raw = false;
- max_point_number = 0;
- underflow_bm.clear();
- overflow_bm.clear();
- segment_size = 0;
- uint64_t mark = in.read_pos();
uint32_t tag = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(tag, in))) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
- if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ||
- tag == FLAG_SCALED_VALUE_OVERFLOW) {
- uint32_t n = 0;
- if (RET_FAIL(common::SerializationUtil::read_var_uint(n, in))) {
- return ret;
+ if (tag == FLAG_SCALED_VALUE_OVERFLOW ||
+ tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
+ uint32_t count = 0;
+ if (RET_FAIL(common::SerializationUtil::read_var_uint(count, in))) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ if (count == 0 || count > 0x7FFFFFFFu) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ const int bm_len = static_cast<int>(count) / 8 + 1;
+ // Guard the allocation: a tiny corrupt page must not trigger a
+ // bitmap-sized allocation the stream cannot possibly back.
+ if (static_cast<uint64_t>(bm_len) >
+ (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ? 2 : 1) *
+ static_cast<uint64_t>(in.remaining_size())) {
+ return common::E_TSFILE_CORRUPTED;
}
- segment_size = static_cast<int>(n);
- int bm_len = segment_size / 8 + 1;
- underflow_bm.resize(static_cast<size_t>(bm_len), 0);
+ meta.has_scaled_bm = true;
+ meta.scaled_bm.resize(static_cast<size_t>(bm_len), 0);
uint32_t read_len = 0;
- if (RET_FAIL(in.read_buf(underflow_bm.data(),
+ if (RET_FAIL(in.read_buf(meta.scaled_bm.data(),
static_cast<uint32_t>(bm_len), read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
- overflow_bm.resize(static_cast<size_t>(bm_len), 0);
- if (RET_FAIL(in.read_buf(overflow_bm.data(),
+ meta.has_raw_bm = true;
+ meta.raw_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (RET_FAIL(in.read_buf(meta.raw_bm.data(),
static_cast<uint32_t>(bm_len),
read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
}
+ meta.page_value_count = static_cast<int>(count);
uint32_t mpn = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(mpn, in))) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
- max_point_number = static_cast<int>(mpn);
- return common::E_OK;
- }
-
- // Distinguish Java maxPointNumber prefix from legacy raw C++ block.
- max_point_number = static_cast<int>(tag);
- if (!looks_like_ts2diff_header(in)) {
- in.set_read_pos(mark);
- is_legacy_raw = true;
+ // No format-level upper bound: Java accepts any maxPointNumber
+ // the varint carries (Math.pow overflow yields Infinity, i.e.
+ // maxPointValue = +inf, and values decode via the raw-bits form).
+ meta.max_point_number = static_cast<int>(mpn);
} else {
- segment_size = 0;
+ // No format-level upper bound on maxPointNumber (see above).
+ meta.max_point_number = static_cast<int>(tag);
+ meta.page_value_count = 0; // unknown until the blocks are decoded
Review Comment:
When using agent tools, please handle intermediate/process information carefully. There is no need to introduce comments explaining something that does not exist in the final result. Please keep the implementation focused on the final outcome.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -319,6 +317,9 @@ class TS2DIFFDecoder : public Decoder {
bit_width_ = 0;
current_index_ = 0;
header_peeked_ = false;
+ header_error_ = false;
+ read_error_ = common::E_OK;
Review Comment:
header_error is assigned but never read.
Delete it or make it work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5473732497
Approved, thanks for all the iterations on this one — feel free to just push the remaining fixes on top.
Two small asks before merge:
- Comments — could you give the whole diff a quick pass? A few comments still carry traces of earlier revisions (the spots I marked are just examples). It'd be great to have them read as if the final version were the only one that ever existed.
- Error codes — I'd prefer the codec-layer codes here (E_DECODE_ERR, matching RLE/Sprintz/RLBE), with short reads propagating the underlying error rather than being rebranded.
And a genuine thank-you for the wire-format doc — it's exactly what this area was missing. Keeping it in sync with future FLOAT/DOUBLE TS_2DIFF changes will make it even more valuable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5421280701
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): handle TS2DIFF float prefixes in batch decode [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5328852641
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5343187750
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): handle TS2DIFF float prefixes in batch decode [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5237723481
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5447300822
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5421076723
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 #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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo commented on code in PR #901:
URL: https://github.com/apache/tsfile/pull/901#discussion_r3869293305
##########
cpp/src/common/allocator/byte_stream.h:
##########
@@ -696,7 +696,18 @@ class ByteStream {
if (UNLIKELY(read_page_ == nullptr)) {
read_page_ = head_.load();
} else if (UNLIKELY((read_pos_ & page_mask_) == 0)) {
- read_page_ = read_page_->next_.load();
+ // At a page boundary the cursor may have been parked here by a
+ // preceding sequential read (read_page_ is the page just
+ // finished, advance one) or by set_read_pos() (read_page_ is
+ // already the boundary page, advancing would skip it). The
+ // two states are indistinguishable, so recompute the page
+ // from the head instead of blindly following next_.
+ Page* p = head_.load();
+ uint64_t page_idx = read_pos_ / page_size_;
+ while (p != nullptr && page_idx-- > 0) {
+ p = p->next_.load();
+ }
+ read_page_ = p;
Review Comment:
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.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -219,80 +219,73 @@ inline bool bitmap_marked(const std::vector<uint8_t>& bm, int idx) {
return (bm[byte_idx] & static_cast<uint8_t>(1u << (idx % 8))) != 0;
}
-inline bool looks_like_ts2diff_header(common::ByteStream& in) {
- int ret = common::E_OK;
- uint64_t probe_mark = in.read_pos();
- int32_t write_index = 0;
- int32_t bit_width = 0;
- if (RET_FAIL(common::SerializationUtil::read_i32(write_index, in)) ||
- RET_FAIL(common::SerializationUtil::read_i32(bit_width, in))) {
- in.set_read_pos(probe_mark);
- return false;
- }
- in.set_read_pos(probe_mark);
- if (write_index < 0 || write_index > 128) {
- return false;
- }
- if (bit_width < 0 || bit_width > 64) {
- return false;
- }
- return true;
-}
+// Page-level FLOAT/DOUBLE metadata, parsed exactly once per page.
+// Layout (see cpp/docs/ts2diff-float-double-wire-format.md):
+// form 1: [maxPointNumber varint]
+// form 2: [Integer.MAX_VALUE][count][scaled bitmap][maxPointNumber]
+// form 3: [Integer.MAX_VALUE-1][count][scaled bitmap][raw bitmap]
+// [maxPointNumber]
+// A leading 0x00 byte is the normal encoding of maxPointNumber = 0 (the
+// Java Ts2Diff builder default), not a legacy marker. Inputs that do not
+// match this grammar are a format error (E_TSFILE_CORRUPTED).
+struct PageMeta {
+ bool has_scaled_bm = false;
+ bool has_raw_bm = false;
+ std::vector<uint8_t> scaled_bm;
+ std::vector<uint8_t> raw_bm;
+ int max_point_number = 0;
+ int page_value_count = 0;
+};
-inline int consume_float_double_ts2diff_prefix(
- common::ByteStream& in, bool& is_legacy_raw, int& max_point_number,
- std::vector<uint8_t>& underflow_bm, std::vector<uint8_t>& overflow_bm,
- int& segment_size) {
+inline int read_page_meta(common::ByteStream& in, PageMeta& meta) {
int ret = common::E_OK;
- is_legacy_raw = false;
- max_point_number = 0;
- underflow_bm.clear();
- overflow_bm.clear();
- segment_size = 0;
- uint64_t mark = in.read_pos();
uint32_t tag = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(tag, in))) {
return ret;
}
- if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW ||
- tag == FLAG_SCALED_VALUE_OVERFLOW) {
- uint32_t n = 0;
- if (RET_FAIL(common::SerializationUtil::read_var_uint(n, in))) {
+ if (tag == FLAG_SCALED_VALUE_OVERFLOW ||
+ tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
+ uint32_t count = 0;
+ if (RET_FAIL(common::SerializationUtil::read_var_uint(count, in))) {
return ret;
}
- segment_size = static_cast<int>(n);
- int bm_len = segment_size / 8 + 1;
- underflow_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (count == 0 || count > 0x7FFFFFFFu) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ const int bm_len = static_cast<int>(count) / 8 + 1;
+ meta.has_scaled_bm = true;
+ meta.scaled_bm.resize(static_cast<size_t>(bm_len), 0);
uint32_t read_len = 0;
- if (RET_FAIL(in.read_buf(underflow_bm.data(),
+ if (RET_FAIL(in.read_buf(meta.scaled_bm.data(),
static_cast<uint32_t>(bm_len), read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
if (tag == FLAG_ORIGINAL_VALUE_OVERFLOW) {
- overflow_bm.resize(static_cast<size_t>(bm_len), 0);
- if (RET_FAIL(in.read_buf(overflow_bm.data(),
+ meta.has_raw_bm = true;
+ meta.raw_bm.resize(static_cast<size_t>(bm_len), 0);
+ if (RET_FAIL(in.read_buf(meta.raw_bm.data(),
static_cast<uint32_t>(bm_len),
read_len)) ||
read_len != static_cast<uint32_t>(bm_len)) {
- return ret;
+ return common::E_TSFILE_CORRUPTED;
}
}
+ meta.page_value_count = static_cast<int>(count);
uint32_t mpn = 0;
if (RET_FAIL(common::SerializationUtil::read_var_uint(mpn, in))) {
return ret;
}
- max_point_number = static_cast<int>(mpn);
- return common::E_OK;
- }
-
- // Distinguish Java maxPointNumber prefix from legacy raw C++ block.
- max_point_number = static_cast<int>(tag);
- if (!looks_like_ts2diff_header(in)) {
- in.set_read_pos(mark);
- is_legacy_raw = true;
+ if (mpn > 100) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ meta.max_point_number = static_cast<int>(mpn);
} else {
- segment_size = 0;
+ if (tag > 100) {
+ return common::E_TSFILE_CORRUPTED;
+ }
+ meta.max_point_number = static_cast<int>(tag);
+ meta.page_value_count = 0; // unknown until the blocks are decoded
}
return common::E_OK;
Review Comment:
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.
##########
cpp/src/encoding/ts2diff_encoder.h:
##########
@@ -553,18 +588,22 @@ int TS2DIFFEncoder<T>::encode_batch(const int64_t* values, uint32_t count,
class FloatTS2DIFFEncoder : public TS2DIFFEncoder<int32_t> {
public:
- FloatTS2DIFFEncoder() : max_point_number_(2), max_point_value_(100.0) {}
+ FloatTS2DIFFEncoder()
+ : max_point_number_(0), // Java Ts2Diff builder default
+ max_point_value_(1.0),
+ page_blocks_(1024, common::MOD_TS2DIFF_OBJ, false) {}
int do_encode(float value, common::ByteStream& out_stream) {
Review Comment:
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.
##########
cpp/src/encoding/ts2diff_decoder.h:
##########
@@ -328,9 +322,30 @@ class TS2DIFFDecoder : public Decoder {
current_index_ != 0);
}
- void read_header(common::ByteStream& in) {
- common::SerializationUtil::read_i32(write_index_, in);
- common::SerializationUtil::read_i32(bit_width_, in);
+ // Reads the 4+4 byte block header. On a truncated stream the old
+ // signature silently kept the previous (stale) write_index_, letting a
+ // caller loop forever re-emitting a phantom block; the failure is
+ // recorded in header_error_ (decode() returns a value, not an error
+ // code) and returned for batch entry points.
+ int read_header(common::ByteStream& in) {
+ int32_t write_index = 0;
+ int32_t bit_width = 0;
+ if (common::SerializationUtil::read_i32(write_index, in) !=
+ common::E_OK ||
+ common::SerializationUtil::read_i32(bit_width, in) !=
+ common::E_OK) {
+ header_error_ = true;
+ return common::E_TSFILE_CORRUPTED;
+ }
+ if (write_index < 0 || write_index > 128 || bit_width < 0 ||
+ bit_width > (int)sizeof(T) * 8) {
+ header_error_ = true;
+ return common::E_TSFILE_CORRUPTED;
+ }
Review Comment:
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.
##########
cpp/docs/ts2diff-float-double-wire-format.md:
##########
@@ -0,0 +1,138 @@
+<!-- Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License. -->
+
+# FLOAT/DOUBLE TS_2DIFF Wire Format (Java Canonical Layout)
+
+This document specifies the canonical on-disk layout of FLOAT/DOUBLE TS_2DIFF
+pages, derived from the Java reference implementation
+(`FloatEncoder`, `FloatDecoder`, `DeltaBinaryEncoder`, `BitMap`).
+The Java layout is the cross-language compatibility boundary. Other layouts
+produced by earlier C++ writers (raw bit-cast, per-block wrapper metadata) are
+implementation artifacts outside the compatibility scope; the C++ decoder
+treats them as a format error.
+
+## Encoding Pipeline
+
+TS_2DIFF encodes integers. Floating-point values go through a wrapper that
+converts each value to an integer, encodes the integers with
+`IntDeltaEncoder` (FLOAT) or `LongDeltaEncoder` (DOUBLE), and emits page-wide
+conversion metadata.
+
+Given `maxPointNumber = mpn` and `maxPointValue = 10^mpn` (`mpn <= 0` implies
+`maxPointValue = 1`), each value maps to one of three stored forms:
+
+| Condition | Stored bits | Decoder action |
+| -------------------------------------- | --------------------------- | ------------------------- |
+| `round(v * 10^mpn)` fits the int type | `round(v * 10^mpn)` | divide by `10^mpn` |
+| scaled overflows but `v` itself fits | `round(v)` | divide by `1` |
+| `v` out of int range, or NaN | `floatToIntBits(v)` / `doubleToLongBits(v)` | restore raw bits |
+
+The three forms are tracked per page as a tri-state flag list
+(`underflowFlags` in Java):
+
+- `true` -> scaled form
+- `false` -> rounded form (scale overflow)
+- `null` -> raw IEEE 754 bits (value overflow or NaN)
+
+## Page Layout
+
+```text
+# Form 1: every value stored in scaled form (no bitmap at all)
+[maxPointNumber varint]
+[TS_2DIFF block 1][TS_2DIFF block 2]...[final block]
+
+# Form 2: at least one value is 'false' (scale overflow), none is 'null'
+[Integer.MAX_VALUE varint] # 0xFF 0xFF 0xFF 0xFF 0x07
+[pageValueCount varint]
+[scaled-bitmap, pageValueCount/8+1 bytes] # marks 'true' entries
+[maxPointNumber varint]
+[TS_2DIFF block 1]...[final block]
+
+# Form 3: at least one value is 'null' (raw bits)
+[Integer.MAX_VALUE-1 varint] # 0xFF 0xFF 0xFF 0xFF 0x06
+[pageValueCount varint]
+[scaled-bitmap, pageValueCount/8+1 bytes] # marks 'true' entries
+[raw-bitmap, pageValueCount/8+1 bytes] # marks 'null' entries
+[maxPointNumber varint]
+[TS_2DIFF block 1]...[final block]
+```
+
+Key invariants:
+
+- `maxPointNumber` appears exactly once per page, before the first integer
+ block (for Forms 2/3 it appears after the bitmaps).
+- The bitmaps cover the entire page, not individual TS_2DIFF blocks. The
+ decoder keeps one page-wide `position` that never resets between blocks;
+ only a page-level `reset()` clears it.
+- Bitmap byte length is always `size/8 + 1`, even when `size % 8 == 0`
+ (`BitMap.getSizeOfBytes`).
+- Bitmap bit order is LSB-first within each byte: position `p` maps to
+ `bits[p / 8] & (1 << (p % 8))`.
+- `pageValueCount` counts all values of the page (across blocks).
+- A first-page byte of `0x00` is the normal encoding of `maxPointNumber = 0`,
+ which is the Java `Ts2Diff` builder default. It is not a legacy marker.
+
+## Integer Block Layout
+
+Identical to the integer TS_2DIFF format (`DeltaBinaryEncoder`):
+
+```text
+[writeIndex int32 BE] # number of values in this block (<= 129)
+[bitWidth int32 BE]
+[block-specific header] # first value; min delta
+[packed data] # writeIndex * bitWidth bits
+```
+
+`BLOCK_DEFAULT_SIZE = 128`: the encoder buffers the first value plus up to 128
+deltas, then flushes a 129-value block. A 300-value page therefore produces
+blocks of 129, 129, 42.
+
+## Encoder Construction
+
+Java `TSEncodingBuilder.Ts2Diff` hard-codes `maxPointNumber = 0` for
+FLOAT/DOUBLE (it does not read `max_point_number` props). Pages produced by
+Java therefore start with `0x00`, and the C++ `FloatTS2DIFFEncoder` /
+`DoubleTS2DIFFEncoder` use the same default. The value stored in the
+stream is self-describing, so files written by other `maxPointNumber`
+values remain readable.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo merged PR #901:
URL: https://github.com/apache/tsfile/pull/901
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): TS_2DIFF float/double maxPointNumber once per page (fixes #910) [tsfile]
Posted by "kkzi (via GitHub)" <gi...@apache.org>.
kkzi commented on PR #901:
URL: https://github.com/apache/tsfile/pull/901#issuecomment-5421066584
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 #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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org