Skip to content

Commit 4364cd6

Browse files
committed
fix(cpp): correct aligned all-null pages and page advancement (#1001)
* fix: continue aligned queries past all-null pages * fix(cpp): write canonical all-null pages and revert Java changes * test(cpp): refresh CLI goldens for compact all-null pages * test(cpp): focus null-page coverage on canonical format
1 parent 2e3ff3b commit 4364cd6

11 files changed

Lines changed: 399 additions & 46 deletions

‎cpp/src/file/restorable_tsfile_io_writer.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ namespace {
5454
const int HEADER_LEN = MAGIC_STRING_TSFILE_LEN + 1; // magic + version
5555
const int BUF_SIZE = 4096;
5656
const unsigned char kTimeChunkTypeMask = 0x80;
57+
const unsigned char kValueChunkTypeMask = 0x40;
5758

5859
// -----------------------------------------------------------------------------
5960
// Self-check helpers: read file, parse chunk header, recover chunk statistics
@@ -263,6 +264,13 @@ static int recover_chunk_statistic(
263264
kTimeChunkTypeMask) != 0;
264265
PageHeader ph;
265266
int ret = ph.deserialize_from(bs, false, chdr.data_type_);
267+
if (ret == common::E_OK && ph.uncompressed_size_ == 0 &&
268+
(static_cast<unsigned char>(chdr.chunk_type_) & kValueChunkTypeMask) !=
269+
0) {
270+
// A single all-null aligned value page has no payload or statistics.
271+
return bs.remaining_size() == 0 ? common::E_OK
272+
: common::E_TSFILE_CORRUPTED;
273+
}
266274
if (ret != common::E_OK || ph.compressed_size_ == 0 ||
267275
bs.remaining_size() < ph.compressed_size_) {
268276
// Align with Java selfCheck behavior: malformed/incomplete page in this

‎cpp/src/reader/aligned_chunk_reader.cc‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -699,11 +699,15 @@ int AlignedChunkReader::decode_time_value_buf_into_tsblock(
699699
value_compressor_->after_uncompress(value_uncompressed_buf_);
700700
value_uncompressed_buf_ = nullptr;
701701
}
702-
if (!prev_value_page_not_finish()) {
703-
value_in_.reset();
704-
}
705702
if (!prev_time_page_not_finish()) {
703+
// The time page determines the aligned value page's end. Even
704+
// after decoding every value, Gorilla can leave a padding byte
705+
// unread, which must not prevent advancing to the next page.
706+
value_in_.reset();
707+
value_decoder_->reset();
706708
time_in_.reset();
709+
} else if (!prev_value_page_not_finish()) {
710+
value_in_.reset();
707711
}
708712
value_page_col_notnull_bitmap_.clear();
709713
value_page_col_notnull_bitmap_.shrink_to_fit();

‎cpp/src/writer/value_chunk_writer.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,10 @@ void ValueChunkWriter::save_first_page_data(
159159
int ValueChunkWriter::write_first_page_data(ByteStream& pages_data,
160160
bool with_statistic) {
161161
int ret = E_OK;
162+
if (first_page_data_.uncompressed_size_ == 0) {
163+
// The empty first page is already complete in chunk_data_.
164+
return ret;
165+
}
162166
if (with_statistic &&
163167
RET_FAIL(first_page_statistic_->serialize_to(pages_data))) {
164168
} else if (RET_FAIL(

‎cpp/src/writer/value_page_writer.cc‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,16 @@ int ValuePageWriter::write_to_chunk(ByteStream& pages_data, bool write_header,
151151
<< pages_data.total_size() << " of chunk_data." << std::endl;
152152
#endif
153153
int ret = E_OK;
154-
if (RET_FAIL(prepare_end_page())) {
155-
return ret;
156-
}
157-
if (RET_FAIL(cur_page_data_.init(col_notnull_bitmap_out_stream_,
158-
value_out_stream_, compressor_, size_))) {
154+
if (statistic_->count_ == 0) {
155+
// Match the aligned empty-page format: a single zero varint, with
156+
// no compressed size, statistics, bitmap, or codec payload.
157+
cur_page_data_ = ValuePageData();
158+
} else {
159+
if (RET_FAIL(prepare_end_page())) {
160+
return ret;
161+
}
162+
ret = cur_page_data_.init(col_notnull_bitmap_out_stream_,
163+
value_out_stream_, compressor_, size_);
159164
}
160165
col_notnull_bitmap_.clear();
161166
size_ = 0;
@@ -164,21 +169,24 @@ int ValuePageWriter::write_to_chunk(ByteStream& pages_data, bool write_header,
164169
if (IS_SUCC(ret) && write_header) {
165170
if (RET_FAIL(SerializationUtil::write_var_uint(
166171
cur_page_data_.uncompressed_size_, pages_data))) {
167-
} else if (RET_FAIL(SerializationUtil::write_var_uint(
172+
} else if (cur_page_data_.uncompressed_size_ != 0 &&
173+
RET_FAIL(SerializationUtil::write_var_uint(
168174
cur_page_data_.compressed_size_, pages_data))) {
169175
}
170176
}
171177
// std::cout << "ValuePageWriter::write_to_chunk after write_header. pos="
172178
// << pages_data.total_size() << std::endl;
173-
if (IS_SUCC(ret) && write_statistic) {
179+
if (IS_SUCC(ret) && write_statistic &&
180+
cur_page_data_.uncompressed_size_ != 0) {
174181
if (RET_FAIL(statistic_->serialize_to(pages_data))) {
175182
}
176183
}
177184
// std::cout << "ValuePageWriter::write_to_chunk after write_stat. pos=" <<
178185
// pages_data.total_size() << std::endl; DEBUG_print_byte_stream("In
179186
// ValuePageWriter::write_to_chunk, before writer page data: pages_data = ",
180187
// pages_data);
181-
if (IS_SUCC(ret) && write_data_to_chunk_data) {
188+
if (IS_SUCC(ret) && write_data_to_chunk_data &&
189+
cur_page_data_.compressed_size_ != 0) {
182190
// DEBUG_hex_dump_buf("cur_page_data_.compressed_buf_ = ",
183191
// cur_page_data_.compressed_buf_, cur_page_data_.compressed_size_);
184192
if (RET_FAIL(pages_data.write_buf(cur_page_data_.compressed_buf_,
Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
#include <gtest/gtest.h>
21+
22+
#include <algorithm>
23+
#include <cstdio>
24+
#include <memory>
25+
#include <string>
26+
#include <tuple>
27+
#include <vector>
28+
29+
#include "common/path.h"
30+
#include "reader/qds_without_timegenerator.h"
31+
#include "reader/tsfile_reader.h"
32+
#include "writer/tsfile_writer.h"
33+
34+
namespace storage {
35+
namespace {
36+
37+
using namespace common;
38+
39+
// Leading and intermediate all-null value pages must not prevent reading
40+
// subsequent pages, regardless of the value codec or write API.
41+
class AlignedNullPageReadTest
42+
: public ::testing::TestWithParam<std::tuple<TSDataType, uint32_t, int>> {
43+
protected:
44+
void SetUp() override {
45+
ASSERT_EQ(libtsfile_init(), E_OK);
46+
saved_page_rows_ = g_config_value_.page_writer_max_point_num_;
47+
saved_block_memory_ = g_config_value_.tsblock_max_memory_;
48+
g_config_value_.page_writer_max_point_num_ = std::get<1>(GetParam());
49+
// The larger pages span multiple result blocks.
50+
g_config_value_.tsblock_max_memory_ = 4096;
51+
file_ = "aligned_null_page_" +
52+
std::to_string(static_cast<int>(std::get<0>(GetParam()))) +
53+
"_" + std::to_string(std::get<1>(GetParam())) + "_" +
54+
std::to_string(std::get<2>(GetParam())) + ".tsfile";
55+
std::remove(file_.c_str());
56+
}
57+
58+
void TearDown() override {
59+
std::remove(file_.c_str());
60+
g_config_value_.page_writer_max_point_num_ = saved_page_rows_;
61+
g_config_value_.tsblock_max_memory_ = saved_block_memory_;
62+
libtsfile_destroy();
63+
}
64+
65+
bool is_null(int row) const {
66+
int page = row / std::get<1>(GetParam());
67+
return page == 0 || page == 2 || page == 3 || page == 6;
68+
}
69+
70+
void add_value(TsRecord& record, int row) {
71+
switch (std::get<0>(GetParam())) {
72+
case INT32:
73+
record.add_point("value", static_cast<int32_t>(1000 + row));
74+
break;
75+
case INT64:
76+
record.add_point("value", static_cast<int64_t>(1000 + row));
77+
break;
78+
case FLOAT:
79+
record.add_point("value", static_cast<float>(1000.25 + row));
80+
break;
81+
case DOUBLE:
82+
record.add_point("value", 1000.25 + row);
83+
break;
84+
case STRING:
85+
record_string_ = "value-" + std::to_string(row);
86+
record.add_point("value", String(record_string_));
87+
break;
88+
default:
89+
FAIL() << "Unexpected type";
90+
}
91+
}
92+
93+
void add_value(Tablet& tablet, int index, int row) {
94+
switch (std::get<0>(GetParam())) {
95+
case INT32:
96+
ASSERT_EQ(tablet.add_value(index, 1,
97+
static_cast<int32_t>(1000 + row)),
98+
E_OK);
99+
break;
100+
case INT64:
101+
ASSERT_EQ(tablet.add_value(index, 1,
102+
static_cast<int64_t>(1000 + row)),
103+
E_OK);
104+
break;
105+
case FLOAT:
106+
ASSERT_EQ(tablet.add_value(index, 1,
107+
static_cast<float>(1000.25 + row)),
108+
E_OK);
109+
break;
110+
case DOUBLE:
111+
ASSERT_EQ(tablet.add_value(index, 1, 1000.25 + row), E_OK);
112+
break;
113+
case STRING: {
114+
std::string value = "value-" + std::to_string(row);
115+
ASSERT_EQ(tablet.add_value(index, 1, value.c_str()), E_OK);
116+
break;
117+
}
118+
default:
119+
FAIL() << "Unexpected type";
120+
}
121+
}
122+
123+
void check_value(Field* field, int row) {
124+
if (is_null(row)) {
125+
EXPECT_EQ(field->type_, NULL_TYPE) << "row=" << row;
126+
return;
127+
}
128+
ASSERT_EQ(field->type_, std::get<0>(GetParam())) << "row=" << row;
129+
switch (field->type_) {
130+
case INT32:
131+
EXPECT_EQ(field->value_.ival_, 1000 + row);
132+
break;
133+
case INT64:
134+
EXPECT_EQ(field->value_.lval_, 1000 + row);
135+
break;
136+
case FLOAT:
137+
EXPECT_FLOAT_EQ(field->value_.fval_, 1000.25f + row);
138+
break;
139+
case DOUBLE:
140+
EXPECT_DOUBLE_EQ(field->value_.dval_, 1000.25 + row);
141+
break;
142+
case STRING:
143+
EXPECT_EQ(field->get_string_value()->to_std_string(),
144+
"value-" + std::to_string(row));
145+
break;
146+
default:
147+
FAIL() << "Unexpected type";
148+
}
149+
}
150+
151+
std::string file_;
152+
std::string record_string_;
153+
uint32_t saved_page_rows_ = 0;
154+
uint32_t saved_block_memory_ = 0;
155+
};
156+
157+
TEST_P(AlignedNullPageReadTest,
158+
ReadsValuesAfterLeadingAndIntermediateNullPages) {
159+
const TSDataType type = std::get<0>(GetParam());
160+
const TSEncoding encoding = type == STRING ? DICTIONARY : GORILLA;
161+
const std::string device = "root.null_pages";
162+
const int rows = 7 * std::get<1>(GetParam());
163+
const int mode = std::get<2>(GetParam());
164+
{
165+
TsFileWriter writer;
166+
ASSERT_EQ(writer.open(file_), E_OK);
167+
std::vector<MeasurementSchema*> schemas{
168+
new MeasurementSchema("anchor", INT64, PLAIN, UNCOMPRESSED),
169+
new MeasurementSchema("value", type, encoding, UNCOMPRESSED)};
170+
ASSERT_EQ(writer.register_aligned_timeseries(device, schemas), E_OK);
171+
for (int row = 0; row < rows;) {
172+
if (mode == 0 || (mode == 2 && row % 3 == 0)) {
173+
TsRecord record(row, device);
174+
record.add_point("anchor", static_cast<int64_t>(row));
175+
if (is_null(row))
176+
record.points_.emplace_back("value");
177+
else
178+
add_value(record, row);
179+
ASSERT_EQ(writer.write_record_aligned(record), E_OK);
180+
++row;
181+
} else {
182+
const int count = std::min(11, rows - row);
183+
auto schema =
184+
std::make_shared<std::vector<MeasurementSchema>>();
185+
schema->emplace_back("anchor", INT64, PLAIN, UNCOMPRESSED);
186+
schema->emplace_back("value", type, encoding, UNCOMPRESSED);
187+
Tablet tablet(device, schema, count);
188+
ASSERT_EQ(tablet.err_code_, E_OK);
189+
for (int index = 0; index < count; ++index) {
190+
ASSERT_EQ(tablet.add_timestamp(index, row + index), E_OK);
191+
ASSERT_EQ(tablet.add_value(
192+
index, 0, static_cast<int64_t>(row + index)),
193+
E_OK);
194+
if (!is_null(row + index))
195+
add_value(tablet, index, row + index);
196+
}
197+
ASSERT_EQ(writer.write_tablet_aligned(tablet), E_OK);
198+
row += count;
199+
}
200+
}
201+
ASSERT_EQ(writer.flush(), E_OK);
202+
ASSERT_EQ(writer.close(), E_OK);
203+
}
204+
205+
TsFileReader reader;
206+
ASSERT_EQ(reader.open(file_), E_OK);
207+
ResultSet* result = nullptr;
208+
std::vector<Path> paths{Path(device + ".anchor"), Path(device + ".value")};
209+
ASSERT_EQ(reader.query(QueryExpression::create(paths, nullptr), result),
210+
E_OK);
211+
int row = 0;
212+
bool more = false;
213+
while (true) {
214+
ASSERT_EQ(result->next(more), E_OK);
215+
if (!more) break;
216+
auto* record = result->get_row_record();
217+
ASSERT_LT(row, rows);
218+
EXPECT_EQ(record->get_timestamp(), row);
219+
EXPECT_EQ(record->get_field(1)->value_.lval_, row);
220+
check_value(record->get_field(2), row);
221+
++row;
222+
}
223+
EXPECT_EQ(row, rows);
224+
reader.destroy_query_data_set(result);
225+
ASSERT_EQ(reader.close(), E_OK);
226+
}
227+
228+
INSTANTIATE_TEST_SUITE_P(
229+
CodecsAndPageBoundaries, AlignedNullPageReadTest,
230+
::testing::Combine(::testing::Values(INT32, INT64, FLOAT, DOUBLE, STRING),
231+
::testing::Values(1u, 7u, 256u),
232+
::testing::Values(0, 1, 2)));
233+
234+
} // namespace
235+
} // namespace storage
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
size_bytes,format_version,model
2-
457,4,table
2+
451,4,table
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"size_bytes":"457","format_version":"4","model":"table"}
1+
{"size_bytes":"451","format_version":"4","model":"table"}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
size_bytes format_version model
2-
457 4 table
2+
451 4 table

0 commit comments

Comments
 (0)