This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new e82d2650e38 [improvement](variant) Assert Variant sparse byte order in
debug builds (#70416)
e82d2650e38 is described below
commit e82d2650e38e57886679faecf0ad5569169c54aa
Author: lihangyu <[email protected]>
AuthorDate: Fri Nov 13 06:18:32 2026 -0800
[improvement](variant) Assert Variant sparse byte order in debug builds
(#70416)
### Background
Variant V2 persists each row's sparse leaves as a dotted-path map, and
every reader merges or
binary-searches those keys by raw path bytes. The writer sorts the
sparse plan (#70001), and
vertical compaction merges the source sparse row with the materialized
subcolumns, so byte order is
a producer obligation all the way to the compacted segment. Nothing
checked it, so a regression
would only surface much later as a query failure (`Duplicate Variant
object key`).
### What problem does this PR solve?
Issue Number: None
Problem Summary:
Without a check, a writer or compaction merge that breaks the order
produces segments whose reads
fail, far from the cause. This change asserts the invariant where it is
relied on and keeps the
assertion out of release builds.
### Details
- **Debug-only checks:** `DCHECK(is_variant_sparse_row_sorted(...))` in
- `StorageMapRowCursor::bind()` (V2 row cursor used by every
hierarchical read),
- the exact sparse-path extractor,
- `SparseColumnMergeIterator::_merge_to` for both the source row and the
merged destination row,
because the merge interleaves sparse cells with materialized subcolumn
paths.
- **Why not repair:** query-time and compaction-time tolerance is
deliberately not added on this
branch. Segments must be byte-ordered; a violation is a producer bug.
- **Performance:** every check is a `DCHECK`, compiled out in release;
queries and compaction keep
the same scan, sort, and allocation behavior as before.
- **Tests:** `VariantAssemblerTest.DebugCheckRejectsUnsortedSparseRows`
(death test) feeds a
two-cell unsorted row and expects the debug check to fire;
`SparseColumnMergeIteratorTest.KeepsByteOrderWhenMergingMaterializedSubcolumn`
merges a
byte-sorted source row with a materialized `a.d` column and asserts the
merged row is
`a-b, a.c, a.d`. The import side is already covered by
`VariantShredderTest.SparseRowsUseByteOrderWithoutChangingStatisticsSelection`.
### Testing
- `./run-be-ut.sh --run
--filter='VariantAssemblerTest.*:BinaryColumnExtractIteratorV2Test.*:VariantShredderTest.*'`
on dev-x (ASAN): 17 tests passed.
- `./run-be-ut.sh --run
--filter='VariantColumnWriterReaderTest.*:IndexStorageVariantCompactionReadTest.*:SparseColumnMergeIteratorTest.*'`
on dev-x (ASAN): 75 passed, 4 pre-existing skips.
- `./run-be-ut.sh --run
--filter='SparseColumnMergeIteratorTest.*:VariantAssemblerTest.*'`
on dev-x (ASAN): 2 passed (merge interleaving + death test).
- Negative check: temporarily removed the writer-side
`std::ranges::sort` from `VariantShredder`
(#70001).
`VariantShredderTest.SparseRowsUseByteOrderWithoutChangingStatisticsSelection`
failed
its byte-order expectation and the new check aborted in
`StorageMapRowCursor::bind`
(`Check failed: is_variant_sparse_row_sorted`), so the assertion does
catch a producer that stops
sorting. The sort was restored afterwards.
- `build-support/check-build-hygiene.sh` passed.
### Release note
None.
### Check List (For Author)
- Behavior changed: No (assertions only, active in debug builds)
- Does this need documentation: No
- Pre-submission status: Ready
### AI Disclosure
- Tool: DeepSeek Harness
- Generated: code, tests, commit message
- Human review: pending
---
be/src/core/column/variant_column_utils.cpp | 9 +++
be/src/core/column/variant_column_utils.h | 5 ++
.../variant/binary_column_extract_iterator.cpp | 2 +
.../variant/sparse_column_merge_iterator.cpp | 10 +++
.../segment/variant/v2/variant_assembler.cpp | 6 ++
.../binary_column_extract_iterator_test.cpp | 92 ++++++++++++++++++++++
6 files changed, 124 insertions(+)
diff --git a/be/src/core/column/variant_column_utils.cpp
b/be/src/core/column/variant_column_utils.cpp
index 920d439a168..9aba8432947 100644
--- a/be/src/core/column/variant_column_utils.cpp
+++ b/be/src/core/column/variant_column_utils.cpp
@@ -35,6 +35,15 @@ size_t find_variant_sparse_path_lower_bound(StringRef path,
const ColumnString&
return start;
}
+bool is_variant_sparse_row_sorted(const ColumnString& sparse_paths, size_t
start, size_t end) {
+ for (size_t i = start + 1; i < end; ++i) {
+ if (sparse_paths.get_data_at(i) < sparse_paths.get_data_at(i - 1)) {
+ return false;
+ }
+ }
+ return true;
+}
+
bool is_variant_jsonb_value_semantically_empty(const JsonbValue* value) {
if (value == nullptr || value->isNull()) {
return true;
diff --git a/be/src/core/column/variant_column_utils.h
b/be/src/core/column/variant_column_utils.h
index 146ae51df4d..ec6fd99cb3e 100644
--- a/be/src/core/column/variant_column_utils.h
+++ b/be/src/core/column/variant_column_utils.h
@@ -30,6 +30,11 @@ struct JsonbValue;
size_t find_variant_sparse_path_lower_bound(StringRef path, const
ColumnString& sparse_paths,
size_t start, size_t end);
+// Readers merge and binary-search sparse keys by raw path bytes, so every
segment must persist a
+// row in that order: the writer sorts the sparse plan (#70001) and compaction
copies cells verbatim.
+// Only debug assertions call this, so release reads and compaction pay
nothing for the check.
+bool is_variant_sparse_row_sorted(const ColumnString& sparse_paths, size_t
start, size_t end);
+
// True when a JSONB value recursively contains no visible scalar payload.
Variant V1 and V2
// readers share this rule when deciding whether a materialized nested value
contributes a path.
bool is_variant_jsonb_value_semantically_empty(const JsonbValue* value);
diff --git a/be/src/storage/segment/variant/binary_column_extract_iterator.cpp
b/be/src/storage/segment/variant/binary_column_extract_iterator.cpp
index 94c19b12c8d..d9764f0e315 100644
--- a/be/src/storage/segment/variant/binary_column_extract_iterator.cpp
+++ b/be/src/storage/segment/variant/binary_column_extract_iterator.cpp
@@ -105,6 +105,8 @@ Status
BinaryColumnExtractIterator::_fill_variant_v2_path(MutableColumnPtr& dst,
return Status::Corruption("Invalid Variant sparse offsets [{}, {})
for {} cells", start,
end, paths->size());
}
+ // Readers binary-search by raw path bytes; a producer that violates
the order is a bug.
+ DCHECK(is_variant_sparse_row_sorted(*paths, start, end));
const size_t pos = find_variant_sparse_path_lower_bound(requested,
*paths, start, end);
// Prefix matches such as a.b belong to another physical path.
if (pos < end && paths->get_data_at(pos) == requested) {
diff --git a/be/src/storage/segment/variant/sparse_column_merge_iterator.cpp
b/be/src/storage/segment/variant/sparse_column_merge_iterator.cpp
index f5824760388..02b683b67ee 100644
--- a/be/src/storage/segment/variant/sparse_column_merge_iterator.cpp
+++ b/be/src/storage/segment/variant/sparse_column_merge_iterator.cpp
@@ -26,6 +26,7 @@
#include "core/column/column.h"
#include "core/column/column_nullable.h"
#include "core/column/subcolumn_tree.h"
+#include "core/column/variant_column_utils.h"
#include "core/data_type/data_type.h"
#include "core/data_type/data_type_nullable.h"
#include "storage/segment/column_reader.h"
@@ -123,6 +124,9 @@ void SparseColumnMergeIterator::_merge_to(MutableColumnPtr&
dst) {
size_t offset = src_serialized_sparse_column_offsets[row - 1];
size_t end = src_serialized_sparse_column_offsets[row];
+ // Compaction only copies cells, and every reader binary-searches by
raw path bytes, so the
+ // source row must already be byte-ordered. Debug-only: release
compaction pays nothing.
+ DCHECK(is_variant_sparse_row_sorted(src_sparse_column_paths, offset,
end));
// Iterator over [path, binary value]
for (size_t i = offset; i != end; ++i) {
const StringRef src_sparse_path_string =
src_sparse_column_paths.get_data_at(i);
@@ -157,6 +161,12 @@ void
SparseColumnMergeIterator::_merge_to(MutableColumnPtr& dst) {
dst_sparse_column_values,
src_path, row);
}
+ // The interleaving above assumes the source sparse cells and the
materialized subcolumns
+ // are byte-ordered. Assert the merged row keeps that invariant for
downstream readers;
+ // debug-only, so release compaction pays nothing.
+ DCHECK(is_variant_sparse_row_sorted(dst_sparse_column_paths,
+
dst_sparse_column_offsets[ssize_t(row) - 1],
+ dst_sparse_column_paths.size()));
// All the sparse columns in this row are null.
dst_sparse_column_offsets.push_back(dst_sparse_column_paths.size());
}
diff --git a/be/src/storage/segment/variant/v2/variant_assembler.cpp
b/be/src/storage/segment/variant/v2/variant_assembler.cpp
index 44833aa0e78..3e734be845b 100644
--- a/be/src/storage/segment/variant/v2/variant_assembler.cpp
+++ b/be/src/storage/segment/variant/v2/variant_assembler.cpp
@@ -90,6 +90,12 @@ struct StorageMapRowCursor {
DORIS_CHECK(values != nullptr);
DORIS_CHECK_EQ(paths->size(), values->size());
DCHECK(rows == 0 || storage_map->offset_at(static_cast<ssize_t>(rows))
== paths->size());
+ // Readers binary-search sparse keys by raw path bytes. The writer
(#70001) and vertical
+ // compaction canonicalize every row, so a non-conforming row reaching
this cursor is a
+ // producer bug; keep the check debug-only so release scans pay
nothing.
+ for (size_t row = 0; row < rows; ++row) {
+ DCHECK(is_variant_sparse_row_sorted(*paths, begin(row),
row_end(row)));
+ }
}
size_t begin(size_t row) const noexcept { return
source->offset_at(static_cast<ssize_t>(row)); }
diff --git a/be/test/storage/segment/binary_column_extract_iterator_test.cpp
b/be/test/storage/segment/binary_column_extract_iterator_test.cpp
index 416b1a01f6e..bf2c5333508 100644
--- a/be/test/storage/segment/binary_column_extract_iterator_test.cpp
+++ b/be/test/storage/segment/binary_column_extract_iterator_test.cpp
@@ -17,6 +17,7 @@
#include "storage/segment/variant/binary_column_extract_iterator.h"
+#include <glog/logging.h>
#include <gtest/gtest.h>
#include <algorithm>
@@ -40,6 +41,8 @@
#include "core/value/jsonb_value.h"
#include "exec/common/variant_util.h"
#include "storage/iterators.h"
+#include "storage/segment/variant/sparse_column_merge_iterator.h"
+#include "storage/segment/variant/v2/variant_assembler.h"
namespace doris::segment_v2 {
namespace {
@@ -789,5 +792,94 @@ TEST(BinaryColumnExtractIteratorV2Test,
DoesNotRescanFrozenPathOrdering) {
EXPECT_EQ(variant_v2_json_at(variant, 0), "7");
}
+// Debug builds must reject a sparse row that violates the byte-order
invariant; the check is a
+// DCHECK so release scans and compaction pay nothing.
+#if DCHECK_IS_ON()
+TEST(VariantAssemblerTest, DebugCheckRejectsUnsortedSparseRows) {
+ auto sparse = variant_util::create_variant_binary_column();
+ auto& map = assert_cast<ColumnMap&>(*sparse);
+ DataTypeInt64 int_type;
+ auto ints = ColumnInt64::create();
+ ints->insert_value(1);
+ ints->insert_value(2);
+ append_storage_cell(map, "a.c", int_type, *ints, 0);
+ append_storage_cell(map, "a-b", int_type, *ints, 1);
+ map.get_offsets().push_back(2);
+
+ variant_v2::VariantAssemblerOptions options;
+ options.storage_map_kind = variant_v2::StorageMapKind::SPARSE;
+ auto assembler = variant_v2::VariantAssembler::create(std::move(options));
+ ASSERT_TRUE(assembler.has_value());
+ EXPECT_DEATH(
+ {
+ ColumnNullable::MutablePtr result;
+ (void)(*assembler)
+ ->assemble({.num_rows = map.size(),
+ .materialized_columns = {},
+ .storage_map = &map},
+ &result);
+ },
+ "is_variant_sparse_row_sorted");
+}
+#endif
+
+// Vertical compaction merges the source sparse row with materialized
subcolumns. The interleaving
+// assumes both sides are byte-ordered, and the merged row is what later
readers see, so it must
+// keep that invariant.
+TEST(SparseColumnMergeIteratorTest,
KeepsByteOrderWhenMergingMaterializedSubcolumn) {
+ auto sparse = variant_util::create_variant_binary_column();
+ auto& map = assert_cast<ColumnMap&>(*sparse);
+ DataTypeInt64 int_type;
+ auto ints = ColumnInt64::create();
+ ints->insert_value(1);
+ ints->insert_value(2);
+ append_storage_cell(map, "a-b", int_type, *ints, 0);
+ append_storage_cell(map, "a.c", int_type, *ints, 1);
+ map.get_offsets().push_back(2);
+
+ auto counters = std::make_shared<SparseReadCounters>();
+ auto cache = std::make_shared<BinaryColumnCache>(
+ std::make_unique<FixedSparseIterator>(std::move(sparse), counters),
+ variant_util::create_variant_binary_column());
+
+ // Materialized path a.d; the dense iterator yields one nullable bigint
row: 3.
+ auto dense_type =
std::make_shared<DataTypeNullable>(std::make_shared<DataTypeInt64>());
+ auto dense_values = ColumnNullable::create(ColumnInt64::create(),
ColumnUInt8::create());
+
assert_cast<ColumnInt64&>(dense_values->get_nested_column()).insert_value(3);
+ dense_values->get_null_map_data().push_back(0);
+ SubstreamReaderTree src_subcolumns;
+ ASSERT_TRUE(src_subcolumns.add(
+ PathInData("a.d"),
+ SubstreamIterator(
+ ColumnNullable::create(ColumnInt64::create(),
ColumnUInt8::create()),
+
std::make_unique<FixedSparseIterator>(std::move(dense_values),
+
std::make_shared<SparseReadCounters>()),
+ dense_type)));
+
+ VariantCompactionPaths path_set_info;
+ OlapReaderStatistics stats;
+ StorageReadOptions read_options(stats);
+ SparseColumnMergeIterator iterator(path_set_info, std::move(cache),
std::move(src_subcolumns),
+ &read_options);
+ ColumnIteratorOptions iterator_options;
+ iterator_options.stats = &stats;
+ ASSERT_TRUE(iterator.init(iterator_options).ok());
+ ASSERT_TRUE(iterator.seek_to_ordinal(0).ok());
+ auto dst = variant_util::create_variant_binary_column();
+ size_t rows = 1;
+ bool has_null = false;
+ ASSERT_TRUE(iterator.next_batch(&rows, dst, &has_null).ok());
+ ASSERT_EQ(dst->size(), 1);
+ const auto& merged = assert_cast<const ColumnMap&>(*dst);
+ const auto& paths = assert_cast<const ColumnString&>(merged.get_keys());
+ std::vector<std::string> actual;
+ const size_t begin = merged.offset_at(0);
+ const size_t end = begin + merged.size_at(0);
+ for (size_t cell = begin; cell < end; ++cell) {
+ actual.push_back(paths.get_data_at(cell).to_string());
+ }
+ EXPECT_EQ(actual, (std::vector<std::string> {"a-b", "a.c", "a.d"}));
+}
+
} // namespace
} // namespace doris::segment_v2
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
