Skip to content
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

generic lookup: pad queries to improve query cachability #10506

Merged
merged 16 commits into from
Jan 29, 2025
+34 −14
Merged

generic lookup: pad queries to improve query cachability #10506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2af0f28
pad queries
yumkam Sep 30, 2024
83255a1
Workaround for connector bug: use first key instead of keys of NULL
yumkam Oct 16, 2024
3d73937
don't exceed MaxKeysInRequest
yumkam Oct 16, 2024
cd95a4a
Fix ut
yumkam Oct 16, 2024
7382239
Merge remote-tracking branch 'origin/main' into streamlookup-pad-queries
yumkam Oct 31, 2024
17badf0
style fixes
yumkam Oct 31, 2024
707ff56
Merge remote-tracking branch 'origin/main' into streamlookup-pad-queries
yumkam Dec 9, 2024
ef56079
move addClause lambda -> AddClause function (cosmetic)
yumkam Dec 24, 2024
01c81be
Merge remote-tracking branch 'origin/main' into streamlookup-pad-queries
yumkam Dec 24, 2024
fe7e284
rename columns -> columnsCount
yumkam Dec 25, 2024
d0437ca
use consistently-cased names
yumkam Dec 25, 2024
a117554
avoid copying
yumkam Dec 25, 2024
e1b4197
(review) *_T* -> *::T*
yumkam Dec 25, 2024
1eb9835
Merge remote-tracking branch 'origin/main' into streamlookup-pad-queries
yumkam Jan 22, 2025
34625a5
fix ut after merge
yumkam Jan 22, 2025
ce0037d
drop idea with null keys and lambda
yumkam Jan 22, 2025
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
12 changes: 12 additions & 0 deletions ydb/library/yql/providers/generic/actors/ut/yql_generic_lookup_actor_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ Y_UNIT_TEST_SUITE(GenericProviderLookupActor) {
.Operand().Equal().Column("optional_id").OptionalValue<ui64>(100).Done().Done()
.Done()
.Done()
.Operand()
.Conjunction()
.Operand().Equal().Column("id").Value<ui64>(2).Done().Done()
.Operand().Equal().Column("optional_id").OptionalValue<ui64>(102).Done().Done()
.Done()
.Done()
.Done()
.Done()
.Done()
Expand Down Expand Up @@ -291,6 +297,12 @@ Y_UNIT_TEST_SUITE(GenericProviderLookupActor) {
.Operand().Equal().Column("optional_id").OptionalValue<ui64>(100).Done().Done()
.Done()
.Done()
.Operand()
.Conjunction()
.Operand().Equal().Column("id").Value<ui64>(2).Done().Done()
.Operand().Equal().Column("optional_id").OptionalValue<ui64>(102).Done().Done()
.Done()
.Done()
.Done()
.Done()
.Done()
Expand Down
36 changes: 22 additions & 14 deletions ydb/library/yql/providers/generic/actors/yql_generic_lookup_actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ namespace NYql::NDq {
if (retriesRemaining) {
const auto retry = RequestRetriesLimit - retriesRemaining;
const auto delay = TDuration::MilliSeconds(1u << retry); // Exponential delay from 1ms to ~0.5s
// <<< TODO tune/tweak
// << TODO tune/tweak
YQL_CLOG(WARN, ProviderGeneric) << "ActorId=" << selfId << " Got retrievable GRPC Error from Connector: " << status.ToDebugString() << ", retry " << (retry + 1) << " of " << RequestRetriesLimit << ", scheduled in " << delay;
--retriesRemaining;
if (status.GRpcStatusCode == grpc::DEADLINE_EXCEEDED) {
Expand Down Expand Up @@ -450,6 +450,19 @@ namespace NYql::NDq {
return result;
}

void AddClause(NConnector::NApi::TPredicate::TDisjunction &disjunction,
ui32 columnsCount, const NUdf::TUnboxedValue& keys) {
NConnector::NApi::TPredicate::TConjunction& conjunction = *disjunction.mutable_operands()->Add()->mutable_conjunction();
for (ui32 c = 0; c != columnsCount; ++c) {
NConnector::NApi::TPredicate::TComparison& eq = *conjunction.mutable_operands()->Add()->mutable_comparison();
eq.set_operation(NConnector::NApi::TPredicate::TComparison::EOperation::TPredicate_TComparison_EOperation_EQ);
eq.mutable_left_value()->set_column(TString(KeyType->GetMemberName(c)));
auto rightTypedValue = eq.mutable_right_value()->mutable_typed_value();
ExportTypeToProto(KeyType->GetMemberType(c), *rightTypedValue->mutable_type());
ExportValueToProto(KeyType->GetMemberType(c), keys.GetElement(c), *rightTypedValue->mutable_value());
}
}

TString FillSelect(NConnector::NApi::TSelect& select) {
auto dsi = LookupSource.data_source_instance();
auto error = TokenProvider->MaybeFillToken(dsi);
Expand All @@ -466,21 +479,16 @@ namespace NYql::NDq {

select.mutable_from()->Settable(LookupSource.table());

NConnector::NApi::TPredicate_TDisjunction disjunction;
for (const auto& [k, _] : *Request) {
NConnector::NApi::TPredicate::TDisjunction disjunction;
for (const auto& [keys, _] : *Request) {
// TODO consider skipping already retrieved keys
// ... but careful, can we end up with zero? TODO
NConnector::NApi::TPredicate_TConjunction conjunction;
for (ui32 c = 0; c != KeyType->GetMembersCount(); ++c) {
NConnector::NApi::TPredicate_TComparison eq;
eq.Setoperation(NConnector::NApi::TPredicate_TComparison_EOperation::TPredicate_TComparison_EOperation_EQ);
eq.mutable_left_value()->Setcolumn(TString(KeyType->GetMemberName(c)));
auto rightTypedValue = eq.mutable_right_value()->mutable_typed_value();
ExportTypeToProto(KeyType->GetMemberType(c), *rightTypedValue->mutable_type());
ExportValueToProto(KeyType->GetMemberType(c), k.GetElement(c), *rightTypedValue->mutable_value());
*conjunction.mutable_operands()->Add()->mutable_comparison() = eq;
}
*disjunction.mutable_operands()->Add()->mutable_conjunction() = conjunction;
AddClause(disjunction, KeyType->GetMembersCount(), keys);
}
auto& keys = Request->begin()->first; // Request is never empty
// Pad query with dummy clauses to improve caching
for (ui32 nRequests = Request->size(); !IsPowerOf2(nRequests) && nRequests < MaxKeysInRequest; ++nRequests) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Может сделать отсечку чтобы на слишком больших значениях уже таким не заниматься? Допустим приходит к нам 2^20+1 элемент всегда, а мы его будем расширять до 2^21. Выглядит как уже большой overhead на таких значениях. Либо же имеет смысл ограничивать шаг начиная с какой-то позиции. Например до 512 степенью двойки идти, а потом уже с шагом 512

Copy link
Collaborator Author

@yumkam yumkam Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

MaxKeysPerRequest ограничен 1k (лимит ydb на ответы). Если бы оно было бы больше, тут категорически НЕЛЬЗЯ было бы идти инкрементами, и обязательно пользоваться только степенями двойки: иначе тут получается O(n^2) вместо O(n), и на больших n это как раз существенно влияет

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cама модель не правильная кмк. Асимптотическая нотация не подходит, так-как тут уже важны константы. Дальше на самом деле важно смотреть на K_i - число ключей в каждом запросе. K_i вполне себе ограничено. Нужно еще учитывать на сколько дороже компиляция станет и на сколько дорого добивать такими фековыми ключами запрос постоянно. Затем размер кеша компиляции не бесконечный и из него вымываются значения. Мое предположение что начиная с какого момента будут очень редкие выбросы и тогда не имеет смысла добавлять накладных расходов на добивание фейковыми значениями как 2x, а достаточно уже взять несколько бакетов по x + k * bucket_size

Сейчас понятно самая тривиальная идея используется, но думаю тут есть что улучшить. Нужно реальных данных насемплировать и по ним посмотреть. Это на уровнее идеи)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ещё есть альтернатива:
Вместо (k1 = ? AND k2 = ?) OR (...) использовать (k1,k2,...) IN ?, где в? биндится к list of tuples. Никакого добивания, однократная компиляция, и минимальный размер запроса.
Cons: требует не вполне тривиальной доработки в коннекторе, апдейта формата протобуфов (сейчас поддерживается только (k1 IN ?) (т.е. один ключ)), и будет работать только на ydb (впрочем, можно зафолбечить на тот же AND OR, только внутри коннектора).

Copy link
Collaborator

Choose a reason for hiding this comment

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

О, это вообще отличная мысль на самом деле

AddClause(disjunction, KeyType->GetMembersCount(), keys);
}
*select.mutable_where()->mutable_filter_typed()->mutable_disjunction() = disjunction;
return {};
Expand Down
Loading