Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions cpp/src/encoding/encoder_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "encoder.h"
#include "encoding/int32_rle_encoder.h"
#include "encoding/int64_rle_encoder.h"
#include "encoding_props.h"
#include "float_sprintz_encoder.h"
#include "gorilla_encoder.h"
#include "int32_sprintz_encoder.h"
Expand Down Expand Up @@ -214,6 +215,27 @@ class EncoderFactory {
return nullptr;
}

static Encoder* alloc_value_encoder(
common::TSEncoding encoding, common::TSDataType data_type,
const std::map<std::string, std::string>& props) {
Encoder* encoder = alloc_value_encoder(encoding, data_type);
if (encoder == nullptr || props.empty() ||
encoding != common::TS_2DIFF ||
(data_type != common::FLOAT && data_type != common::DOUBLE)) {
return encoder;
}
const int precision = detail::max_point_number(props);
if (precision == 2) return encoder; // Already the encoder default.
Comment on lines +227 to +228

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed by reproduction. On the published PR head, the added regressions fail in 32 zero-only writer variants plus one encoder boundary test. Both FLOAT and DOUBLE are affected, at precisions 2147483646 and 2147483647. The leading precision is interpreted as a bitmap marker, and C++ rejects the following zero count.

I have tested a local fix that forces the existing scaled-value bitmap wrapper for these two precisions, retaining an all-scaled bitmap and writing the actual precision after it. Both existing decoders understand that position as a precision value. The local fix passed 87 focused tests, 992 Windows tests and 1013 Linux tests. All 64 added C++ boundary pages were readable by Java; 8 additional C++ zero-only files passed both own and Java reads.

One compatibility detail: the current Java encoder itself emits unreadable zero-only pages for these two precisions (16 raw pages and 8 full files reproduced). Copying those bytes would reproduce the bug; the explicit existing wrapper is required. These invalid Java outputs are recorded as failures, not counted as interoperability passes.

The fix has not been pushed yet: our performance gate detected a small reproducible FLOAT record slowdown (125 paired runs, median ratio 1.0172, 95% CI [1.0062, 1.0244]). I am investigating that before publishing the revision and will update this thread with the validated commit and evidence.

AI disclosure: This response, reproduction and reported local validation were prepared/performed by OpenAI Codex.

AI 使用说明: 本回复、复现及报告的本地验证由 OpenAI Codex 编写或执行。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated update for 1e8d230805d5823f1e0cd7a82af4e815510080e0: the fix is now pushed to this PR.

The two reserved precision values now force the existing scaled-value bitmap wrapper in both floating-point flush paths, even when every value is scaled zero. The actual precision follows the count/bitmap and is read as a value by both existing decoders. Empty pages still emit nothing. The new zero-only regressions failed 33 tests before this correction and pass afterward.

The final revision also makes Java rounding exact at half-neighbors and large integers: the old floor(x + 0.5) can round twice. An IEEE-significand implementation matches Java on 1,088,975 deterministic random/edge inputs for both saturated int32 and int64 results (the legacy int64 floor approximation differs on 251). Actual NaN inputs keep the existing raw-bit handling; zero multiplied by an infinite scale rounds to zero. The record schema-check helpers borrow the caller-owned DeviceID instead of copying its shared_ptr on every record; they retain no reference and public APIs/object layouts are unchanged.

Final local validation:

  • 88 focused tests; Windows 993 passed / 3 skipped / 10 existing disabled; Linux 1014 passed / 1 skipped / 10 existing disabled. Linux enables all six optional codecs.
  • Original 122 page cases and another 122 cases with rounding boundaries: bidirectional decode and exact Java/C++ bytes all pass. Complete files: 64 original own/cross reads plus 16 additional C++ zero-file own/Java reads pass. Native compressed fixtures: C++ reads 135 Java files and Java reads 117 C++ files.
  • All 64 additional C++ boundary pages are readable by Java. The 16 Java-generated ambiguous zero-only pages remain explicitly recorded as invalid Java inputs; they fail Java's own reader too. Of 48 valid Java boundary pages, 36 match raw bytes and 12 one-value pages differ only in the pre-existing unused zero-delta minimum field.
  • Changed executable lines: 97/97 (100.00%); corresponding branches 44.78%, with no branch exclusions or ignored parse errors. Format and patch checks pass.

The earlier performance signal was investigated rather than omitted. Final paired latency ratios (same benchmark source/configuration, 125 retained pairs for FLOAT record including the fixed-size confirmation; 25 pairs for each other workload):

  • float_nonaligned_record_default: 1.0000, 95% CI [0.9941, 1.0049].
  • double_nonaligned_tablet_default: 0.9812, 95% CI [0.9662, 0.9901].
  • float_aligned_tablet_precision_2: 0.9822, 95% CI [0.9756, 0.9868].
  • double_aligned_record_precision_2: 0.9999, 95% CI [0.9910, 1.0044].

No statistically distinguishable slowdown was observed in this final matrix; the report retains earlier slower versions and all measurements. This is evidence for these workloads, not a claim about every possible workload.

Validation report · Logs, sources, coverage and measurements

AI disclosure: This response, implementation, tests and reported local validation were prepared/performed by OpenAI Codex.

AI 使用说明: 本回复、实现、测试及报告的本地验证由 OpenAI Codex 编写或执行。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanitizer follow-up in 75592b89d1523a96090e8bca1dcb4cd2180162e5: both Ubuntu ASan jobs found a lifetime error in my new zero-page test. DecoderFactory::free does not call the decoder destructor, so its bitmap vectors leaked 424 bytes in 32 allocations. The test now uses scoped concrete decoders. No production code changes. The original test reproduces the same leak locally; the revised 7 encoding tests and full Linux Debug ASan suite (1014 passed, 1 skipped, all six codecs enabled) pass with leak detection on. Windows Release: 993 passed. Production source and prior measured artifacts are SHA-256 identical, preserving the previously reported compatibility, coverage and performance evidence. Supplementary report · Logs and reproduction.

AI disclosure: This correction, response and reported validation were prepared/performed by OpenAI Codex.

AI 使用说明: 本修正、回复及报告的验证由 OpenAI Codex 编写或执行。

if (data_type == common::FLOAT) {
static_cast<FloatTS2DIFFEncoder*>(encoder)->set_max_point_number(
precision);
} else {
static_cast<DoubleTS2DIFFEncoder*>(encoder)->set_max_point_number(
precision);
}
return encoder;
}

static void free(Encoder* encoder) {
if (encoder == nullptr) {
return;
Expand Down
93 changes: 93 additions & 0 deletions cpp/src/encoding/encoding_props.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* 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.
*/

#ifndef ENCODING_ENCODING_PROPS_H
#define ENCODING_ENCODING_PROPS_H

#include <cstdint>
#include <map>
#include <string>

namespace storage {
namespace detail {

// Integer.parseInt uses Character.digit(char, 10): BMP decimal digits are
// accepted, but supplementary digits (UTF-16 surrogate pairs) are not.
inline int precision_digit(const std::string& text, size_t& offset) {
const auto first = static_cast<unsigned char>(text[offset++]);
if (first < 0x80) {
return first >= '0' && first <= '9' ? first - '0' : -1;
}
int trailing;
uint32_t code;
if (first >= 0xC2 && first <= 0xDF) {
trailing = 1;
code = first & 0x1F;
} else if (first >= 0xE0 && first <= 0xEF) {
trailing = 2;
code = first & 0x0F;
} else {
return -1;
}
if (offset + trailing > text.size()) return -1;
for (int i = 0; i < trailing; ++i) {
const auto next = static_cast<unsigned char>(text[offset++]);
if ((next & 0xC0) != 0x80) return -1;
code = (code << 6) | (next & 0x3F);
}
if ((trailing == 2 && code < 0x800) || (code >= 0xD800 && code <= 0xDFFF))
return -1;
static const uint16_t zeroes[] = {
0x0660, 0x06F0, 0x07C0, 0x0966, 0x09E6, 0x0A66, 0x0AE6, 0x0B66, 0x0BE6,
0x0C66, 0x0CE6, 0x0D66, 0x0DE6, 0x0E50, 0x0ED0, 0x0F20, 0x1040, 0x1090,
0x17E0, 0x1810, 0x1946, 0x19D0, 0x1A80, 0x1A90, 0x1B50, 0x1BB0, 0x1C40,
0x1C50, 0xA620, 0xA8D0, 0xA900, 0xA9D0, 0xA9F0, 0xAA50, 0xABF0, 0xFF10};
for (uint16_t zero : zeroes) {
if (code >= zero && code <= zero + 9U) return code - zero;
}
return -1;
}

inline int max_point_number(const std::map<std::string, std::string>& props) {
const auto property = props.find("max_point_number");
if (property == props.end()) return 2;
const auto& text = property->second;
size_t offset = 0;
bool negative = false;
if (!text.empty() && (text[0] == '-' || text[0] == '+')) {
negative = text[0] == '-';
++offset;
}
// A fresh Java Ts2Diff builder retains zero after NumberFormatException.
if (offset == text.size()) return 0;
const uint32_t limit = negative ? 2147483648U : 2147483647U;
uint32_t value = 0;
while (offset < text.size()) {
const int digit = precision_digit(text, offset);
if (digit < 0 || value > (limit - digit) / 10) return 0;
value = value * 10 + digit;
}
// Negative precision falls back to Java's default floatPrecision (2).
return negative && value != 0 ? 2 : static_cast<int>(value);
}

} // namespace detail
} // namespace storage

#endif // ENCODING_ENCODING_PROPS_H
64 changes: 43 additions & 21 deletions cpp/src/encoding/ts2diff_encoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -578,26 +578,39 @@ int TS2DIFFEncoder<T>::encode_batch(const int64_t* values, uint32_t count,

// Java Math.round(x) == floor(x + 0.5) (ties go towards +infinity),
// unlike std::lround's ties-away-from-zero (-2.5 -> -2, not -3).
// Math.round saturates at the integer limits; a plain static_cast of the
// boundary value (e.g. INT64_MAX rounding up from below, or 2^63 passing
// the <= (double)INT64_MAX gate) would be undefined behaviour, so the
// conversion clamps first. The 64-bit upper bound is compared against
// 2^63 rather than (double)INT64_MAX, which rounds up to 2^63 and would
// let exactly 2^63 through; the lower limits are exactly representable.
// Round the IEEE significand using integer arithmetic: ties go toward
// positive infinity, NaN becomes zero, and out-of-range results saturate.
// Adding 0.5 before floor can itself round (just below 0.5, or above 2^52).
template <typename Int>
inline Int java_round(double x) {
const double r = std::floor(x + 0.5);
if (sizeof(Int) < 8) {
if (r > static_cast<double>(std::numeric_limits<Int>::max())) {
return std::numeric_limits<Int>::max();
}
} else if (r >= 9223372036854775808.0) { // 2^63
return std::numeric_limits<Int>::max();
FORCE_INLINE Int java_round(double x) {
const uint64_t bits = static_cast<uint64_t>(common::double_to_long(x));
const uint64_t fraction = bits & 0x000FFFFFFFFFFFFFULL;
const unsigned exponent = static_cast<unsigned>((bits >> 52) & 0x7FF);
const bool negative = (bits >> 63) != 0;
if (exponent < 1022) return 0; // Magnitude below one half.
if (exponent >= 1023 + std::numeric_limits<Int>::digits) {
if (exponent == 0x7FF && fraction != 0) return 0;
return negative ? std::numeric_limits<Int>::min()
: std::numeric_limits<Int>::max();
}
const uint64_t significand = fraction | (uint64_t{1} << 52);
const int shift = 1075 - static_cast<int>(exponent);
uint64_t rounded = 0;
if (shift > 0) {
const uint64_t remainder = significand & ((uint64_t{1} << shift) - 1);
const uint64_t threshold = (uint64_t{1} << (shift - 1)) + negative;
rounded = (significand >> shift) + (remainder >= threshold);
} else {
// Here the value is already integral; the range gate bounds this
// left shift to 0..10 for int64_t (unreachable for int32_t).
rounded = significand << -shift;
}
if (r < static_cast<double>(std::numeric_limits<Int>::min())) {
return std::numeric_limits<Int>::min();
if (rounded > static_cast<uint64_t>(std::numeric_limits<Int>::max())) {
return negative ? std::numeric_limits<Int>::min()
: std::numeric_limits<Int>::max();
}
return static_cast<Int>(r);
const Int result = static_cast<Int>(rounded);
return negative ? -result : result;
}

class FloatTS2DIFFEncoder : public TS2DIFFEncoder<int32_t> {
Expand Down Expand Up @@ -635,7 +648,7 @@ class FloatTS2DIFFEncoder : public TS2DIFFEncoder<int32_t> {
int encode(double value, common::ByteStream& out_stream);

private:
int32_t convert_float_to_int(float value) {
FORCE_INLINE int32_t convert_float_to_int(float value) {
const double scaled = static_cast<double>(value) * max_point_value_;
if (scaled > static_cast<double>(std::numeric_limits<int32_t>::max()) ||
scaled < static_cast<double>(std::numeric_limits<int32_t>::min())) {
Expand Down Expand Up @@ -711,7 +724,7 @@ class DoubleTS2DIFFEncoder : public TS2DIFFEncoder<int64_t> {
int encode(double value, common::ByteStream& out_stream);

private:
int64_t convert_double_to_long(double value) {
FORCE_INLINE int64_t convert_double_to_long(double value) {
const double scaled = value * max_point_value_;
if (scaled > static_cast<double>(std::numeric_limits<int64_t>::max()) ||
scaled < static_cast<double>(std::numeric_limits<int64_t>::min())) {
Expand Down Expand Up @@ -887,7 +900,13 @@ FORCE_INLINE int FloatTS2DIFFEncoder::flush(common::ByteStream& out_stream) {
return common::E_OK;
}
const int num_values = static_cast<int>(underflow_flags_.size());
if (has_overflow()) {
// The two largest precisions collide with the leading overflow tags.
// Always wrap them, even for all-scaled zeroes, so the actual precision
// appears after the bitmap where both Java and C++ read it as a value.
if (has_overflow() ||
max_point_number_ >=
static_cast<int>(
ts2diff_java_detail::FLAG_ORIGINAL_VALUE_OVERFLOW)) {
std::vector<uint8_t> scaled_bitmap(
static_cast<size_t>(num_values / 8 + 1), 0);
std::vector<uint8_t> raw_bits_bitmap(
Expand Down Expand Up @@ -957,7 +976,10 @@ FORCE_INLINE int DoubleTS2DIFFEncoder::flush(common::ByteStream& out_stream) {
return common::E_OK;
}
const int num_values = static_cast<int>(underflow_flags_.size());
if (has_overflow()) {
if (has_overflow() ||
max_point_number_ >=
static_cast<int>(
ts2diff_java_detail::FLAG_ORIGINAL_VALUE_OVERFLOW)) {
std::vector<uint8_t> scaled_bitmap(
static_cast<size_t>(num_values / 8 + 1), 0);
std::vector<uint8_t> raw_bits_bitmap(
Expand Down
11 changes: 9 additions & 2 deletions cpp/src/writer/chunk_writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,19 @@ int ChunkWriter::init(const ColumnSchema& col_schema) {

int ChunkWriter::init(const std::string& measurement_name, TSDataType data_type,
TSEncoding encoding, CompressionType compression_type) {
return init(measurement_name, data_type, encoding, compression_type,
std::map<std::string, std::string>());
}

int ChunkWriter::init(const std::string& measurement_name, TSDataType data_type,
TSEncoding encoding, CompressionType compression_type,
const std::map<std::string, std::string>& props) {
int ret = E_OK;
chunk_statistic_ = StatisticFactory::alloc_statistic(data_type);
if (chunk_statistic_ == nullptr) {
return E_OOM;
} else if (RET_FAIL(
page_writer_.init(data_type, encoding, compression_type))) {
} else if (RET_FAIL(page_writer_.init(data_type, encoding, compression_type,
props))) {
} else if (IS_NULL((first_page_statistic_ =
StatisticFactory::alloc_statistic(data_type)))) {
ret = E_OOM;
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/writer/chunk_writer.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ class ChunkWriter {
int init(const std::string& measurement_name, common::TSDataType data_type,
common::TSEncoding encoding,
common::CompressionType compression_type);
int init(const std::string& measurement_name, common::TSDataType data_type,
common::TSEncoding encoding,
common::CompressionType compression_type,
const std::map<std::string, std::string>& props);
void reset();
void destroy();

Expand Down
9 changes: 8 additions & 1 deletion cpp/src/writer/page_writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,19 @@ int PageData::init(ByteStream& time_bs, ByteStream& value_bs,
/* ================ PageWriter ================ */
int PageWriter::init(TSDataType data_type, TSEncoding encoding,
CompressionType compression) {
return init(data_type, encoding, compression,
std::map<std::string, std::string>());
}

int PageWriter::init(TSDataType data_type, TSEncoding encoding,
CompressionType compression,
const std::map<std::string, std::string>& props) {
int ret = E_OK;
data_type_ = data_type;
if (nullptr == (time_encoder_ = EncoderFactory::alloc_time_encoder())) {
ret = E_OOM;
} else if (nullptr == (value_encoder_ = EncoderFactory::alloc_value_encoder(
encoding, data_type))) {
encoding, data_type, props))) {
ret = E_OOM;
} else if (nullptr ==
(statistic_ = StatisticFactory::alloc_statistic(data_type))) {
Expand Down
6 changes: 6 additions & 0 deletions cpp/src/writer/page_writer.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
#ifndef WRITER_PAGE_WRITER_H
#define WRITER_PAGE_WRITER_H

#include <map>
#include <string>

#include "common/allocator/byte_stream.h"
#include "common/allocator/my_string.h"
#include "common/statistic.h"
Expand Down Expand Up @@ -104,6 +107,9 @@ class PageWriter {
is_inited_(false) {}
int init(common::TSDataType data_type, common::TSEncoding encoding,
common::CompressionType compression);
int init(common::TSDataType data_type, common::TSEncoding encoding,
common::CompressionType compression,
const std::map<std::string, std::string>& props);
~PageWriter() { destroy(); }
// reset statistic_, time_out_stream_, value_out_stream_
void reset();
Expand Down
17 changes: 10 additions & 7 deletions cpp/src/writer/tsfile_writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ int TsFileWriter::register_aligned_timeseries(
MeasurementSchema* ms = new MeasurementSchema(
measurement_schema.measurement_name_, measurement_schema.data_type_,
measurement_schema.encoding_, measurement_schema.compression_type_);
ms->props_ = measurement_schema.props_;
int ret = register_aligned_timeseries(device_id,
std::vector<MeasurementSchema*>{ms});
if (ret != E_OK) {
Expand Down Expand Up @@ -354,6 +355,7 @@ int TsFileWriter::register_timeseries(
MeasurementSchema* ms = new MeasurementSchema(
measurement_schema.measurement_name_, measurement_schema.data_type_,
measurement_schema.encoding_, measurement_schema.compression_type_);
ms->props_ = measurement_schema.props_;
int ret = register_timeseries(device_id, ms);
if (ret != E_OK) {
delete ms;
Expand Down Expand Up @@ -508,7 +510,7 @@ std::shared_ptr<TableSchema> TsFileWriter::get_table_schema(

template <typename MeasurementNamesGetter>
int TsFileWriter::do_check_schema(
std::shared_ptr<IDeviceID> device_id,
const std::shared_ptr<IDeviceID>& device_id,
MeasurementNamesGetter& measurement_names,
SimpleVector<ChunkWriter*>& chunk_writers,
SimpleVector<common::TSDataType>& data_types) {
Expand All @@ -533,9 +535,9 @@ int TsFileWriter::do_check_schema(
MeasurementSchema* ms = ms_iter->second;
if (IS_NULL(ms->chunk_writer_)) {
ms->chunk_writer_ = new ChunkWriter;
ret = ms->chunk_writer_->init(ms->measurement_name_,
ms->data_type_, ms->encoding_,
ms->compression_type_);
ret = ms->chunk_writer_->init(
ms->measurement_name_, ms->data_type_, ms->encoding_,
ms->compression_type_, ms->props_);
if (IS_SUCC(ret)) {
chunk_writers.push_back(ms->chunk_writer_);
} else {
Expand Down Expand Up @@ -569,7 +571,8 @@ int TsFileWriter::ensure_aligned_value_chunk_writer(
}
int ret = value_chunk_writer->init(
measurement_schema->measurement_name_, measurement_schema->data_type_,
measurement_schema->encoding_, measurement_schema->compression_type_);
measurement_schema->encoding_, measurement_schema->compression_type_,
measurement_schema->props_);
if (RET_FAIL(ret)) {
delete value_chunk_writer;
return (ret == E_OOM) ? ret : common::E_INVALID_ARG;
Expand All @@ -580,7 +583,7 @@ int TsFileWriter::ensure_aligned_value_chunk_writer(

template <typename MeasurementNamesGetter>
int TsFileWriter::do_check_schema_aligned(
std::shared_ptr<IDeviceID> device_id,
const std::shared_ptr<IDeviceID>& device_id,
MeasurementNamesGetter& measurement_names,
storage::TimeChunkWriter*& time_chunk_writer,
common::SimpleVector<storage::ValueChunkWriter*>& value_chunk_writers,
Expand Down Expand Up @@ -696,7 +699,7 @@ int TsFileWriter::do_check_schema_table(
ms->value_chunk_writer_ = new ValueChunkWriter;
ret = ms->value_chunk_writer_->init(
ms->measurement_name_, ms->data_type_, ms->encoding_,
ms->compression_type_);
ms->compression_type_, ms->props_);
if (IS_SUCC(ret)) {
value_chunk_writers.push_back(ms->value_chunk_writer_);
} else {
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/writer/tsfile_writer.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,14 @@ class TsFileWriter {

template <typename MeasurementNamesGetter>
int do_check_schema(
std::shared_ptr<IDeviceID> device_id,
const std::shared_ptr<IDeviceID>& device_id,
MeasurementNamesGetter& measurement_names,
common::SimpleVector<storage::ChunkWriter*>& chunk_writers,
common::SimpleVector<common::TSDataType>& data_types);

template <typename MeasurementNamesGetter>
int do_check_schema_aligned(
std::shared_ptr<IDeviceID> device_id,
const std::shared_ptr<IDeviceID>& device_id,
MeasurementNamesGetter& measurement_names,
storage::TimeChunkWriter*& time_chunk_writer,
common::SimpleVector<storage::ValueChunkWriter*>& value_chunk_writers,
Expand Down
Loading
Loading