Skip to content

24-1 Fix B-Tree histograms comparator result overflow #15820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 17, 2025
Merged

24-1 Fix B-Tree histograms comparator result overflow #15820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions ydb/core/tablet_flat/flat_page_btree_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <ydb/core/base/defs.h>
#include <ydb/core/scheme/scheme_tablecell.h>
#include <util/generic/bitmap.h>
#include "flat_page_base.h"
#include "flat_page_label.h"
Expand Down Expand Up @@ -282,6 +283,23 @@ namespace NKikimr::NTable::NPage {
return Count() > 0;
}

void Describe(IOutputStream& out, const TKeyCellDefaults& keyDefaults) const
{
out << '{';

auto iter = Iter();
for (TPos pos : xrange(iter.Count())) {
if (pos != 0) {
out << ", ";
}
TString value;
DbgPrintValue(value, iter.Next(), keyDefaults.BasicTypes()[pos]);
out << value;
}

out << '}';
}

private:
const TIsNullBitmap* IsNullBitmap;
TColumns Columns;
Expand Down
4 changes: 2 additions & 2 deletions ydb/core/tablet_flat/flat_part_dump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ namespace {

void TDump::Key(TCellsRef key, const TPartScheme &scheme) noexcept
{
Out << "(";
Out << "{";

for (auto off : xrange(key.size())) {
TString str;
Expand All @@ -291,7 +291,7 @@ namespace {
Out << (off ? ", " : "") << str;
}

Out << ")";
Out << "}";
}

void TDump::BTreeIndexNode(const TPart &part, NPage::TBtreeIndexNode::TChild meta, ui32 level) noexcept
Expand Down
25 changes: 24 additions & 1 deletion ydb/core/tablet_flat/flat_part_slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ void TSlice::Describe(IOutputStream& out) const noexcept
{
out << (FirstInclusive ? '[' : '(');
out << FirstRowId;
out << ',';
out << ", ";
if (LastRowId != Max<TRowId>()) {
out << LastRowId;
} else {
Expand All @@ -192,6 +192,15 @@ void TSlice::Describe(IOutputStream& out) const noexcept
out << (LastInclusive ? ']' : ')');
}

void TSlice::Describe(IOutputStream& out, const TKeyCellDefaults& keyDefaults) const
{
out << "{rows: ";
Describe(out);
out << " keys: ";
TBounds::Describe(out, keyDefaults);
out << "}";
}

void TSlices::Describe(IOutputStream& out) const noexcept
{
bool first = true;
Expand All @@ -206,6 +215,20 @@ void TSlices::Describe(IOutputStream& out) const noexcept
out << (first ? "}" : " }");
}

void TSlices::Describe(IOutputStream& out, const TKeyCellDefaults& keyDefaults) const
{
bool first = true;
out << "{ ";
for (const auto& bounds : *this) {
if (first)
first = false;
else
out << ", ";
bounds.Describe(out, keyDefaults);
}
out << (first ? "}" : " }");
}

void TSlices::Validate() const noexcept
{
TRowId lastEnd = 0;
Expand Down
3 changes: 3 additions & 0 deletions ydb/core/tablet_flat/flat_part_slice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ namespace NTable {
}

void Describe(IOutputStream& out) const noexcept;
void Describe(IOutputStream& out, const TKeyCellDefaults& keyDefaults) const;

/**
* Returns true if first row of a is less than first row of b
Expand Down Expand Up @@ -328,6 +329,8 @@ namespace NTable {

void Describe(IOutputStream& out) const noexcept;

void Describe(IOutputStream& out, const TKeyCellDefaults& keyDefaults) const;

/**
* Validate slices are correct, crash otherwise
*/
Expand Down
2 changes: 1 addition & 1 deletion ydb/core/tablet_flat/flat_part_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,7 +969,7 @@ namespace NTable {

TPos it;
for (it = 0; it < Key.size(); it++) {
if (int cmp = CompareTypedCells(PrevPageLastKey[it], Key[it], layout.KeyTypes[it])) {
if (CompareTypedCells(PrevPageLastKey[it], Key[it], layout.KeyTypes[it]) != 0) {
break;
}
}
Expand Down
89 changes: 53 additions & 36 deletions ydb/core/tablet_flat/flat_stat_table_btree_index_histogram.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class TTableHistogramBuilderBtreeIndex {
{
}

TString ToString() const noexcept {
TString ToString(const TKeyCellDefaults &keyDefaults) const {
return TStringBuilder()
<< "Part: " << Part->Label.ToString()
<< " PageId: " << PageId
Expand All @@ -57,8 +57,8 @@ class TTableHistogramBuilderBtreeIndex {
<< " EndRowId: " << EndRowId
<< " BeginDataSize: " << BeginDataSize
<< " EndDataSize: " << EndDataSize
<< " BeginKey: " << BeginKey.Count()
<< " EndKey: " << EndKey.Count()
<< " BeginKey: " << NFmt::Do(BeginKey, keyDefaults)
<< " EndKey: " << NFmt::Do(EndKey, keyDefaults)
<< " State: " << (ui32)State;
}

Expand Down Expand Up @@ -130,15 +130,17 @@ class TTableHistogramBuilderBtreeIndex {
};

struct TEvent {
TCellsIterable Key;
bool IsBegin;
TNodeState* Node;
bool IsBegin;

TString ToString() const noexcept {
TString ToString(const TKeyCellDefaults &keyDefaults) const {
return TStringBuilder()
<< Node->ToString()
<< " IsBegin: " << IsBegin
<< " Key: " << Key.Count();
<< "IsBegin: " << IsBegin
<< " " << Node->ToString(keyDefaults);
}

const TCellsIterable& GetKey() const {
return IsBegin ? Node->BeginKey : Node->EndKey;
}
};

Expand All @@ -149,7 +151,7 @@ class TTableHistogramBuilderBtreeIndex {
return Compare(a, b) > 0;
}

i8 Compare(const TEvent& a, const TEvent& b) const noexcept {
int Compare(const TEvent& a, const TEvent& b) const {
// events go in order:
// - Key = {}, IsBegin = true
// - ...
Expand All @@ -161,13 +163,16 @@ class TTableHistogramBuilderBtreeIndex {
// - ...
// - Key = {}, IsBegin = false

if (a.Key && b.Key) { // compare by keys
auto cmp = CompareKeys(a.Key, b.Key, KeyDefaults);
// end goes before begin in order to
// close previous node before open the next one

if (a.GetKey() && b.GetKey()) { // compare by keys
auto cmp = CompareKeys(a.GetKey(), b.GetKey(), KeyDefaults);
if (cmp != 0) {
return cmp;
}
// keys are the same, compare by begin flag, end events first:
return Compare(a.IsBegin ? 1 : -1, b.IsBegin ? 1 : -1);
return Compare(a.IsBegin ? +1 : -1, b.IsBegin ? +1 : -1);
}

// category = -1 for Key = { }, IsBegin = true
Expand All @@ -177,14 +182,14 @@ class TTableHistogramBuilderBtreeIndex {
}

private:
static i8 GetCategory(const TEvent& a) noexcept {
if (a.Key) {
static int GetCategory(const TEvent& a) {
if (a.GetKey()) {
return 0;
}
return a.IsBegin ? -1 : +1;
}

static i8 Compare(i8 a, i8 b) noexcept {
static int Compare(int a, int b) {
if (a < b) return -1;
if (a > b) return +1;
return 0;
Expand Down Expand Up @@ -226,6 +231,9 @@ class TTableHistogramBuilderBtreeIndex {

for (auto index : xrange(Subset.Flatten.size())) {
auto& part = Subset.Flatten[index];
if (part.Slices) {
LOG_BUILD_STATS("slicing part " << part->Label << ": " << NFmt::Do(*part.Slices, KeyDefaults));
}
auto& meta = part->IndexPages.GetBTree({});
TCellsIterable beginKey = EmptyKey;
if (part.Slices && part.Slices->front().FirstKey.GetCells()) {
Expand All @@ -235,7 +243,7 @@ class TTableHistogramBuilderBtreeIndex {
if (part.Slices && part.Slices->back().LastKey.GetCells()) {
endKey = MakeCellsIterableKey(part.Part.Get(), part.Slices->back().LastKey);
}
LoadedStateNodes.emplace_back(part.Part.Get(), meta.GetPageId(), meta.LevelCount, 0, meta.GetRowCount(), 0, meta.GetDataSize(), beginKey, endKey);
LoadedStateNodes.emplace_back(part.Part.Get(), meta.GetPageId(), meta.LevelCount, 0, meta.GetRowCount(), 0, meta.GetTotalDataSize(), beginKey, endKey);
ready &= SlicePart(*part.Slices, LoadedStateNodes.back());
}

Expand All @@ -261,13 +269,13 @@ class TTableHistogramBuilderBtreeIndex {

if (it == slices.end() || node.EndRowId <= it->BeginRowId() || it->EndRowId() <= node.BeginRowId) {
// skip the node
LOG_BUILD_STATS("slicing node " << node.ToString() << " => skip");
LOG_BUILD_STATS("slicing node " << node.ToString(KeyDefaults) << " => skip");
return true;
}

if (it->BeginRowId() <= node.BeginRowId && node.EndRowId <= it->EndRowId()) {
// take the node
LOG_BUILD_STATS("slicing node " << node.ToString() << " => take");
LOG_BUILD_STATS("slicing node " << node.ToString(KeyDefaults) << " => take");
AddFutureEvents(node);
return true;
}
Expand All @@ -278,17 +286,20 @@ class TTableHistogramBuilderBtreeIndex {
// can't split, decide by node.EndRowId - 1
// TODO: decide by non-empty slice and node intersection, but this requires size calculation changes too
if (it->Has(node.EndRowId - 1)) {
LOG_BUILD_STATS("slicing node " << node.ToString() << " => take root");
LOG_BUILD_STATS("slicing node " << node.ToString(KeyDefaults) << " => take leaf");
// the slice may start after node begin, shift the node begin to make it more sensible
node.BeginRowId = it->BeginRowId();
node.BeginKey = MakeCellsIterableKey(node.Part, it->FirstKey);
AddFutureEvents(node);
} else {
LOG_BUILD_STATS("slicing node " << node.ToString() << " => skip root");
LOG_BUILD_STATS("slicing node " << node.ToString(KeyDefaults) << " => skip leaf");
}
return true;
}

bool ready = true;

LOG_BUILD_STATS("slicing node " << node.ToString() << " => split");
LOG_BUILD_STATS("slicing node " << node.ToString(KeyDefaults) << " => split");
const auto addNode = [&](TNodeState& child) {
ready &= SlicePart(slices, child);
};
Expand Down Expand Up @@ -341,10 +352,10 @@ class TTableHistogramBuilderBtreeIndex {
<< " openedSortedByRowCount: " << openedSortedByRowCount.size()
<< " openedSortedByDataSize: " << openedSortedByDataSize.size()
<< " FutureEvents: " << FutureEvents.size()
<< " currentKeyPointer: " << currentKeyPointer.ToString());
<< " currentKeyPointer: " << currentKeyPointer.ToString(KeyDefaults));

auto processEvent = [&](const TEvent& event) {
LOG_BUILD_STATS("processing event " << event.ToString());
LOG_BUILD_STATS("processing event " << event.ToString(KeyDefaults));
Y_DEBUG_ABORT_UNLESS(NodeEventKeyGreater.Compare(event, currentKeyPointer) <= 0, "Can't process future events");
if (event.IsBegin) {
if (event.Node->Open(openedRowCount, openedDataSize)) {
Expand All @@ -370,7 +381,7 @@ class TTableHistogramBuilderBtreeIndex {
// TODO: skip all closed nodes and don't process them here
// TODO: don't compare each node key and replace it with parentNode.Seek(currentKeyPointer)
auto cmp = NodeEventKeyGreater.Compare(event, currentKeyPointer);
LOG_BUILD_STATS("adding event " << (i32)cmp << " " << event.ToString());
LOG_BUILD_STATS("adding event " << (i32)cmp << " " << event.ToString(KeyDefaults));
if (cmp <= 0) { // event happened
processEvent(event);
if (cmp == 0) {
Expand All @@ -381,8 +392,8 @@ class TTableHistogramBuilderBtreeIndex {
}
};
const auto addNode = [&](TNodeState& node) {
addEvent(TEvent{node.BeginKey, true, &node});
addEvent(TEvent{node.EndKey, false, &node});
addEvent(TEvent{&node, true});
addEvent(TEvent{&node, false});
};

// may safely skip current key pointer and go further only if at the next iteration
Expand All @@ -395,7 +406,7 @@ class TTableHistogramBuilderBtreeIndex {
openedSortedByRowCount.pop();

LOG_BUILD_STATS("loading node by row count trigger"
<< node->ToString()
<< node->ToString(KeyDefaults)
<< " closedRowCount: " << closedRowCount
<< " openedRowCount: " << openedRowCount
<< " nextHistogramRowCount: " << nextHistogramRowCount);
Expand All @@ -413,7 +424,7 @@ class TTableHistogramBuilderBtreeIndex {
openedSortedByDataSize.pop();

LOG_BUILD_STATS("loading node by data size trigger"
<< node->ToString()
<< node->ToString(KeyDefaults)
<< " closedDataSize: " << closedDataSize
<< " openedDataSize: " << openedDataSize
<< " nextHistogramDataSize: " << nextHistogramDataSize);
Expand All @@ -439,7 +450,7 @@ class TTableHistogramBuilderBtreeIndex {
<< " openedSortedByRowCount: " << openedSortedByRowCount.size()
<< " openedSortedByDataSize: " << openedSortedByDataSize.size()
<< " FutureEvents: " << FutureEvents.size()
<< " currentKeyPointer: " << currentKeyPointer.ToString());
<< " currentKeyPointer: " << currentKeyPointer.ToString(KeyDefaults));

// add current key pointer to a histogram if we either:
// - failed to split opened nodes and may exceed a next histogram bucket value (plus its gaps)
Expand All @@ -449,7 +460,7 @@ class TTableHistogramBuilderBtreeIndex {
// - minus size of all nodes that start at current key pointer
// - plus half of size of all ohter opened nodes (as they exact position is unknown)
// also check that current key pointer value is > then last presented value in a histogram
if (currentKeyPointer.Key) {
if (currentKeyPointer.GetKey()) {
if (nextHistogramRowCount != Max<ui64>()) {
if (closedRowCount + openedRowCount > nextHistogramRowCount + RowCountResolutionGap || closedRowCount > nextHistogramRowCount - RowCountResolutionGap) {
ui64 currentKeyRowCountOpens = 0;
Expand All @@ -461,7 +472,7 @@ class TTableHistogramBuilderBtreeIndex {
Y_ABORT_UNLESS(currentKeyRowCountOpens <= openedRowCount);
ui64 currentKeyPointerRowCount = closedRowCount + (openedRowCount - currentKeyRowCountOpens) / 2;
if ((stats.RowCountHistogram.empty() ? 0 : stats.RowCountHistogram.back().Value) < currentKeyPointerRowCount && currentKeyPointerRowCount < stats.RowCount) {
AddKey(stats.RowCountHistogram, currentKeyPointer.Key, currentKeyPointerRowCount);
AddKey(stats.RowCountHistogram, currentKeyPointer.GetKey(), currentKeyPointerRowCount);
nextHistogramRowCount = Max(currentKeyPointerRowCount + 1, nextHistogramRowCount + RowCountResolution);
if (nextHistogramRowCount + RowCountResolutionGap > stats.RowCount) {
nextHistogramRowCount = Max<ui64>();
Expand All @@ -480,7 +491,7 @@ class TTableHistogramBuilderBtreeIndex {
Y_ABORT_UNLESS(currentKeyDataSizeOpens <= openedDataSize);
ui64 currentKeyPointerDataSize = closedDataSize + (openedDataSize - currentKeyDataSizeOpens) / 2;
if ((stats.DataSizeHistogram.empty() ? 0 : stats.DataSizeHistogram.back().Value) < currentKeyPointerDataSize && currentKeyPointerDataSize < stats.DataSize.Size) {
AddKey(stats.DataSizeHistogram, currentKeyPointer.Key, currentKeyPointerDataSize);
AddKey(stats.DataSizeHistogram, currentKeyPointer.GetKey(), currentKeyPointerDataSize);
nextHistogramDataSize = Max(currentKeyPointerDataSize + 1, nextHistogramDataSize + DataSizeResolution);
if (nextHistogramDataSize + DataSizeResolutionGap > stats.DataSize.Size) {
nextHistogramDataSize = Max<ui64>();
Expand All @@ -507,7 +518,7 @@ class TTableHistogramBuilderBtreeIndex {
return true;
}

void AddKey(THistogram& histogram, TCellsIterable& key, ui64 value) {
void AddKey(THistogram& histogram, const TCellsIterable& key, ui64 value) {
TVector<TCell> keyCells;

// add columns that are present in the part:
Expand Down Expand Up @@ -555,8 +566,14 @@ class TTableHistogramBuilderBtreeIndex {
}

void AddFutureEvents(TNodeState& node) {
FutureEvents.push(TEvent{node.BeginKey, true, &node});
FutureEvents.push(TEvent{node.EndKey, false, &node});
auto cmp = NodeEventKeyGreater.Compare(TEvent{&node, true}, TEvent{&node, false});
LOG_BUILD_STATS("adding node future events " << (i32)cmp << " " << node.ToString(KeyDefaults));
if (node.GetRowCount() > 1) {
Y_DEBUG_ABORT_UNLESS(cmp < 0);
}

FutureEvents.push(TEvent{&node, true});
FutureEvents.push(TEvent{&node, false});
}

private:
Expand Down
Loading
Loading