Fix prefixed vector index with PK columns (#18196) #18889
Conversation
|
🟢 |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
01afe3f to
accb48e
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
accb48e to
ede4851
Compare
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
2 times, most recently
from
82462a3 to
1220559
Compare
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
2 times, most recently
from
bb83424 to
1220559
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
1220559 to
7ce4e87
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
kungasc
left a comment
kungasc
left a comment
There was a problem hiding this comment.
Предлагаю:
- разделить методы для префиксного и нет kmeans
- у каждого метода подписать схему (колонки) таблиц из и в
- добавить несколько простых тестов уровня kqp для префиксного и непрефиксного индекса с проверкой содержимого таблиц и схемы
| .AddNonNullableColumn("data", EPrimitiveType::String); | ||
| } | ||
| tableBuilder.SetPrimaryKeyColumns({"pk"}); | ||
| if (suffixPk) { |
There was a problem hiding this comment.
тест что ты добавил окей пусть будет, но может ещё простеньких тестов сделать несколько штук как в тикете?
то есть CREATE TABLE + индекс и проверить содержимое всех трех таблиц и их схему
(и не для префиксного тоже бы)
без сплитов, просто базовую функциональность
их раньше ещё читать нельзя было, а теперь можно
There was a problem hiding this comment.
короче говоря посмотрел, вроде дело хорошее, но выглядит как будто надо это отдельно делать, тут-то именно багфикс. а остальные кейсы, где не префиксный и т.п. - вроде и так другими тестами проверяются
| } | ||
|
|
||
| void AddRowBuildToBuild(TBufferData& buffer, TClusterId parent, TArrayRef<const TCell> key, TArrayRef<const TCell> row, ui32 prefixColumns) | ||
| void AddRowBuildToBuild(TBufferData& buffer, TClusterId parent, TArrayRef<const TCell> key, TArrayRef<const TCell> row, ui32 rowPkColumns) |
There was a problem hiding this comment.
давай два метода сделаем, с дефолтными значениями и вычитаниями какой-то ужас
и напиши плиз комментариями (ты почти уже написал) почему например делаем row.Slice(0, row.size()-rowPkColumns)
возможно это выражение лучше в переменную вынести и там подписать будет что почему выкидываем
There was a problem hiding this comment.
аналогично. не два метода, а явные параметры
| } | ||
|
|
||
| void AddRowBuildToPosting(TBufferData& buffer, TClusterId parent, TArrayRef<const TCell> key, TArrayRef<const TCell> row, ui32 dataPos, ui32 prefixColumns) | ||
| void AddRowBuildToPosting(TBufferData& buffer, TClusterId parent, TArrayRef<const TCell> key, TArrayRef<const TCell> row, ui32 dataPos, ui32 rowPkColumns) |
There was a problem hiding this comment.
тут я переделал по-другому - посмотри. должно быть норм. два метода по идее больше не нужно :-)
я явным образом передаю теперь туда несколько .Slice(), чтобы в самих хелперах не было выбора колонок, а оно просто брало готовые списки: первичный ключ, колонки данных. должно быть глобально понятнее
|
|
||
| std::shared_ptr<NTxProxy::TUploadTypes> MakeOutputTypes(const TUserTable& table, NKikimrTxDataShard::EKMeansState uploadState, | ||
| const TProtoStringType& embedding, const google::protobuf::RepeatedPtrField<TProtoStringType>& data, | ||
| ui32 prefixColumns) |
There was a problem hiding this comment.
тут наверно тоже два метода бы, но не уверен, посмотри сам как они логически вызываются
и рядом со всеми методами нарисуй плиз схему строки
There was a problem hiding this comment.
да вот тут мне кажется можно и оставить, тут относительно простая логика - ну типа если даны явно колонки первичного ключа то берём их из pkColumns...
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
7ce4e87 to
5cf5453
Compare
|
⚪ |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
5cf5453 to
e4d3cef
Compare
|
⚪ |
|
⚪ |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
e4d3cef to
52aa930
Compare
|
⚪ |
|
⚪ |
vitalif
force-pushed
the
fix-prefixed-index-with-pk-columns
branch
from
52aa930 to
1490f2d
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
в итоге ещё упростил - сделал 1 хелпер AddRowToData вместо 4 AddRowBuildToBuild/AddRowBuildToPosting/AddRowMainToBuild/AddRowMainToPosting |
3 participants
Changelog category
Description for reviewers
When some columns of the prefix are also part of the primary key, prefix table PK isn't equal to
<prefix>+<pk>anymore. But the previous implementation relies on that and constructs the subsequent build table PK by replacing<prefix_count>columns in the beginning of the prefix table PK with__ydb_parent.The only correct way to fix it is to pass original table's PK column list in PrefixKMeansRequests explicitly. Then it can request them in scan tags in the right order and use them in the PK.