Repository navigation
[C++] Fix serialization of measurement schema properties - #1018
Open
jixuan1989 wants to merge 1 commit into
Open
jixuan1989 wants to merge 1 commit into
jixuan1989 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The localized fix preserves error propagation and has focused regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes #1017 by making C++ measurement schema serialization write non-empty property payloads, matching Java’s wire format.
Changes:
- Enters the property-writing loop after successfully writing the property count.
- Adds eight regression tests covering wire bytes, property variants, stream boundaries, table schemas, and truncated input.
| File | Description |
|---|---|
cpp/test/common/measurement_schema_serialization_test.cc |
Adds eight serialization regression tests. |
cpp/src/common/schema.h |
Corrects the property-count success condition. |
Review effort: Balanced
jixuan1989
added a commit
to jixuan1989/tsfile-apache
that referenced
this pull request
Oct 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A C++ measurement schema with non-empty properties returned
E_OKafter writing only the property count. Its key/value payload was missing, so reading the schema (or table metadata containing it) failed. For the issue reproducer, this changes the incomplete 22-byte output to the same 47 bytes produced by Java.Enter the property loop after the count is written successfully. Add eight regressions for Java wire bytes, embedded NUL bytes on write, empty/multiple/UTF-8/long properties, page boundaries, consecutive schemas, table schemas and truncated input. Seven regressions fail against the original implementation; all eight pass with this fix.
Why the condition must be
RET_SUCCBoth macros evaluate their argument once and assign its return code to the local
ret. Ignoring the branch-prediction hints, their conditions are:The body guarded by the property-count write contains the normal work of serializing each property key and value. It must run when that write succeeds.
RET_FAILconditionRET_SUCCconditionE_OKwith an incomplete payloadretThe corrected outer control flow is equivalent to:
The existing inner
RET_FAILguards still handle key/value failures: they save the error inret, andIS_FAIL(ret)breaks the loop. The earlier header-fieldRET_FAILchecks also remain appropriate because their empty failure branches stop theelse ifchain. Their bodies serve a different purpose from this property-writing loop.An empty map still writes a zero count and executes zero loop iterations. For the reported non-empty input, the count is one; the original serializer omits the 25-byte key/value payload, producing 22 bytes instead of Java's 47. This one-condition change emits the missing payload while preserving the format and error-return convention.
Fixes #1017.
Validation:
MeasurementSchema96.88%, completeschema.h96.14%. Corresponding branch coverage: serializer 55.56%, class 59.26%, header 64.75%. Byte-identical source/build header paths are merged; error branches are not excluded. Allocation/write failures were not fault-injected; their propagation above is established by control-flow inspection.git diff --checkpass.Detailed validation report · Raw evidence and validation scripts
A separate existing
read_strlimitation truncates embedded NUL bytes in both original and fixed builds. The serializer's NUL-containing output is verified against Java. Encoder precision wiring and string reading remain separate follow-up work.Query performance backfill (2026-10-18)
Added 16 native table-query scenarios over aligned/non-aligned chunks, row/batch APIs, 1/4 projected fields, time/TAG filters, sparse NULLs and parallel settings; application FIELD references include full native materialization cost. Two devices × 32,768 rows × four fields, 32 position-balanced process repetitions and three full checked queries/process. Open/initialization, cached schema lookup, setup, first decoded/accepted row, full-result latency/throughput, per-query p50/p95 and whole-process peak working set are retained. Query samples are clustered by process; no cold-cache, P99 or universal no-regression claim.
No scenario triggers the confirmation policy in the measured corpus. Valid empty-property products are byte-identical. The original nonempty-property product is malformed and is retained as an untimed expected failure; both readers instead time the valid revised nonempty-property file. This does not present invalid output as a fast baseline. Existing write/coverage/bidirectional compatibility results retain their original scope.
One initial ordering attempt was stopped after its self-check found unequal execution positions; those samples are preserved and excluded from conclusions. The corrected order gives each variant every position equally. Every native returned cell/NULL/timestamp and Python row/checksum expectation is checked. Java independently audits all valid C++ products and actual value-page codecs/counts/precision; five new helper sanitizer scenarios pass with Linux leak detection.
Query backfill report · Sources, actual products, raw timing arrays and controls
查询性能补测(2026-10-18)
新增 16 个原生表查询场景,覆盖对齐/非对齐 chunk、行/批接口、1/4 个投影字段、时间/TAG 过滤、稀疏 NULL 和并行配置;应用层 FIELD 参考包含完整原生读取与物化成本。每文件 2 个设备 × 32,768 行 × 4 个字段,32 轮执行位置均衡的独立进程,每进程完整消费并核对 3 次查询。保留打开/初始化、缓存 schema 查询、查询构建、首个解码/接受行、完整延迟/吞吐、逐查询 p50/p95 和进程峰值工作集。统计按进程聚类,不声称冷缓存、P99 或所有场景均无退化。
本次 16 个场景均未触发复查条件;有效的空属性前后产物字节一致。旧版本非空属性文件本身损坏,作为预期失败保留且不计时;改用修复后的有效非空属性文件分别计时两个 reader,没有把无效输出当作快速基线。原有写入、覆盖率与双向兼容证据保留原先范围。
最初一次执行顺序存在位置不均衡,已中止并保留数据,不用于结论;修正后各版本在所有执行位置出现次数相等。原生程序逐单元格核验值/NULL/时间戳,Python 独立核验行数与校验和;Java 独立核验全部有效 C++ 产物及实际页编码/数量/精度。新增辅助程序的 5 个 Linux sanitizer 场景通过,开启泄漏检测。
查询补测报告 · 源码、实际文件、原始计时与控制实验
AI disclosure: The implementation, tests and description in this pull request were written by an AI coding agent (OpenAI Codex), which also performed the reported local validation.
AI 使用说明: 本 PR 的代码修改、测试和正文由 AI 编程代理(OpenAI Codex)编写,文中报告的本地验证也由该 AI 代理执行。