Fix: vector index description wasn't persisted #11969

MBkkt · 2024-11-25T17:14:59Z

No description provided.

github-actions · 2024-11-25T20:01:37Z

⚪ 2024-11-25 20:01:37 UTC Pre-commit check linux-x86_64-release-asan for 0ab8793 has started.
⚪ 2024-11-25 20:01:47 UTC Artifacts will be uploaded here
⚪ 2024-11-25 20:04:53 UTC ya make is running...
🟡 2024-11-25 21:40:03 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
9146	9067	0	20	13	46

🟢 2024-11-25 21:40:49 UTC Build successful.
🟢 2024-11-25 21:41:18 UTC ydbd size 4.9 GiB changed* by +89.3 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `7644097`	merge: `0ab8793`	diff	diff %
ydbd size	5 285 400 112 Bytes	5 285 491 512 Bytes	+89.3 KiB	+0.002%
ydbd stripped size	1 360 266 704 Bytes	1 360 289 200 Bytes	+22.0 KiB	+0.002%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2024-11-25T20:04:58Z

⚪ 2024-11-25 20:04:57 UTC Pre-commit check linux-x86_64-relwithdebinfo for 0ab8793 has started.
⚪ 2024-11-25 20:05:32 UTC Artifacts will be uploaded here
⚪ 2024-11-25 20:09:08 UTC ya make is running...
🟢 2024-11-25 21:15:31 UTC Tests successful.

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
24890	22059	0	0	2719	112

🟢 2024-11-25 21:17:41 UTC Build successful.
🟢 2024-11-25 21:18:00 UTC ydbd size 2.5 GiB changed* by +83.3 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `a328b30`	merge: `0ab8793`	diff	diff %
ydbd size	2 690 474 568 Bytes	2 690 559 848 Bytes	+83.3 KiB	+0.003%
ydbd stripped size	482 239 248 Bytes	482 249 136 Bytes	+9.7 KiB	+0.002%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

ijon · 2024-11-26T14:26:22Z

ydb/core/tx/schemeshard/schemeshard__operation_consistent_copy_tables.cpp

@@ -35,6 +35,7 @@ NKikimrSchemeOp::TModifyScheme CreateIndexTask(NKikimr::NSchemeShard::TTableInde
    operation->SetName(dst.LeafName());

    operation->SetType(indexInfo->Type);
+    Y_ENSURE(indexInfo->Type != NKikimrSchemeOp::EIndexType::EIndexTypeGlobalVectorKmeansTree);


Y_ENSURE throws an exception and that is not appropriate for the schemeshard. At least, exceptions are not used in the schemeshard. (And there is a little sense in introducing new way of acting on code assumptions violation as a sidecar of a bugfix).

There is a problem, I don't like the idea of schemeshard aborting if something is written incorrectly and we pass the wrong index type in some schemeshard operation, I prefer abort this operation instead.

Do you have any suggestions on what I can replace this "ensure" code with?

ijon · 2024-11-26T17:35:53Z

ydb/core/tx/schemeshard/schemeshard_schema.h

@@ -1043,9 +1044,10 @@ struct Schema : NIceDb::Schema {
        struct AlterVersion :   Column<3, NScheme::NTypeIds::Uint64> {};
        struct IndexType :      Column<4, NScheme::NTypeIds::Uint32> { using Type = NKikimrSchemeOp::EIndexType; static constexpr Type Default = NKikimrSchemeOp::EIndexTypeInvalid; };
        struct State :          Column<5, NScheme::NTypeIds::Uint32> { using Type = NKikimrSchemeOp::EIndexState; static constexpr Type Default = NKikimrSchemeOp::EIndexStateInvalid; };
+        struct Description :    Column<6, NScheme::NTypeIds::String> {};


Please add a tail comment with the type that is stored in that field.

Not all fields that are intended to store serialized protobuf messages are marked in this way, but many are, and that is very helpful.

ijon · 2024-11-26T17:52:52Z

ydb/core/tx/schemeshard/schemeshard__init.cpp

@@ -1007,7 +1008,8 @@ struct TSchemeShard::TTxInit : public TTransactionBase<TSchemeShard> {
            }
            while (!rowSet.EndOfSet()) {
                const auto pathId = Self->MakeLocalId(TLocalPathId(rowSet.GetValue<Schema::TableIndex::PathId>()));
-                tableIndexes.push_back(MakeTableIndexRec<Schema::TableIndex>(pathId, rowSet));
+                auto& back = tableIndexes.emplace_back(MakeTableIndexRec<Schema::TableIndex>(pathId, rowSet));
+                std::get<4>(back) = rowSet.GetValue<Schema::TableIndex::Description>();


I would have expected either having different variants of MakeTableIndexRec() for a different number of columns or a single variadic MakeTableIndexRec().
But not a special treatment like that.

~~I think current code is more compact, but ok, I will change it~~
Changed

ijon · 2024-11-26T18:00:16Z

ydb/core/tx/schemeshard/schemeshard_info_types.h

@@ -2371,11 +2371,16 @@ struct TTableIndexInfo : public TSimpleRefCount<TTableIndexInfo> {
    using EType = NKikimrSchemeOp::EIndexType;
    using EState = NKikimrSchemeOp::EIndexState;

-    TTableIndexInfo(ui64 version, EType type, EState state)
+    TTableIndexInfo(ui64 version, EType type, EState state, std::string_view description)


Is there a reason to use std::string_view here?

In my head it's commonly best way to pass something like const string reference.
Also ParseFromString uses string_view (abseil but it's kind of same):

bool ParseFromString(y_absl::string_view data);

Do you have some other preference? What and why?

const TString&.
We don't store original argument value in the object and don't construct TTableIndexInfo from anything but TString value, so string_view doesn't bring any benefits here.

ijon · 2024-11-26T18:19:07Z

ydb/core/tx/schemeshard/schemeshard_info_types.h

@@ -2391,8 +2396,20 @@ struct TTableIndexInfo : public TSimpleRefCount<TTableIndexInfo> {
        return result;
    }

+    TString DescriptionToStr() const {
+        return std::visit([](const auto& v) {
+            if constexpr (requires { v.SerializeAsString(); }) {


Can you explain why this check is necessary? I guess, we expect all variants of SpecializedIndexDescription to be of protobuf message type.

We expect monostate for global secondary indexes, but I agree that is maybe better to check this instead

ijon · 2024-11-26T18:41:47Z

ydb/core/tx/schemeshard/schemeshard_info_types.h

@@ -2391,8 +2396,20 @@ struct TTableIndexInfo : public TSimpleRefCount<TTableIndexInfo> {
        return result;
    }

+    TString DescriptionToStr() const {


ToStr has a misleading connotation. According to our codebase it doesn't have a meaning of "encode value to a sequence of bytes" but rather "make something printable to be viewed by a person".
I would recommend using something more accurate: SerializedDescription or something.

Ok, changed to SerializeDescription, not SerializedDescription because I want it to be clear that it's not just getter

github-actions · 2024-11-26T19:19:53Z

⚪ 2024-11-26 19:19:52 UTC Pre-commit check linux-x86_64-release-asan for 9b00599 has started.
⚪ 2024-11-26 19:20:16 UTC Artifacts will be uploaded here
⚪ 2024-11-26 19:23:10 UTC ya make is running...
⚫ 2024-11-26 20:21:58 UTC Check cancelled

github-actions · 2024-11-26T19:20:34Z

⚪ 2024-11-26 19:20:33 UTC Pre-commit check linux-x86_64-relwithdebinfo for 9b00599 has started.
⚪ 2024-11-26 19:20:44 UTC Artifacts will be uploaded here
⚪ 2024-11-26 19:23:40 UTC ya make is running...
🟡 2024-11-26 20:21:02 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
15515	14004	0	2	1396	113

🟢 2024-11-26 20:22:06 UTC ydbd size 2.5 GiB changed* by +39.5 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `f4d6825`	merge: `9b00599`	diff	diff %
ydbd size	2 694 619 000 Bytes	2 694 659 408 Bytes	+39.5 KiB	+0.001%
ydbd stripped size	482 600 784 Bytes	482 604 560 Bytes	+3.7 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}
⚫ 2024-11-26 20:22:07 UTC Check cancelled

github-actions · 2024-11-26T20:22:52Z

⚪ 2024-11-26 20:22:52 UTC Pre-commit check linux-x86_64-release-asan for 4ddb7dc has started.
⚪ 2024-11-26 20:23:14 UTC Artifacts will be uploaded here
⚪ 2024-11-26 20:26:08 UTC ya make is running...
🟡 2024-11-26 21:32:36 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
9105	9023	0	20	9	53

🟢 2024-11-26 21:33:22 UTC Build successful.
🟢 2024-11-26 21:33:50 UTC ydbd size 4.9 GiB changed* by +78.0 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `f4d6825`	merge: `4ddb7dc`	diff	diff %
ydbd size	5 293 855 752 Bytes	5 293 935 600 Bytes	+78.0 KiB	+0.002%
ydbd stripped size	1 361 407 056 Bytes	1 361 420 880 Bytes	+13.5 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2024-11-26T20:25:50Z

⚪ 2024-11-26 20:25:50 UTC Pre-commit check linux-x86_64-relwithdebinfo for 4ddb7dc has started.
⚪ 2024-11-26 20:26:01 UTC Artifacts will be uploaded here
⚪ 2024-11-26 20:28:57 UTC ya make is running...
🟡 2024-11-26 21:20:28 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
15516	14007	0	1	1395	113

⚪ 2024-11-26 21:21:45 UTC ya make is running... (failed tests rerun, try 2)
🟢 2024-11-26 21:33:34 UTC Tests successful.

Test history | Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
112 (only retried tests)	6	0	0	0	106

🟢 2024-11-26 21:33:41 UTC Build successful.
🟢 2024-11-26 21:34:00 UTC ydbd size 2.5 GiB changed* by +45.5 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `f4d6825`	merge: `4ddb7dc`	diff	diff %
ydbd size	2 694 619 000 Bytes	2 694 665 584 Bytes	+45.5 KiB	+0.002%
ydbd stripped size	482 600 784 Bytes	482 605 712 Bytes	+4.8 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

Fix: vector index description wasn't persisted

9a14735

MBkkt requested a review from ijon November 25, 2024 17:14

This comment was marked as outdated.

Sign in to view

Vector index description persisted: add test

6964950

MBkkt force-pushed the mbkkt/fix-vector-index-desc branch from 6ffde9b to 6964950 Compare November 25, 2024 17:33

This comment was marked as outdated.

Sign in to view

Canonize

f6eff60

MBkkt self-assigned this Nov 26, 2024

ijon reviewed Nov 26, 2024

View reviewed changes

Apply review suggestion

500d47f

Apply review suggestion

25bf47f

ijon approved these changes Nov 26, 2024

View reviewed changes

MBkkt merged commit cb1675a into ydb-platform:main Nov 27, 2024
10 checks passed

GrigoriyPA mentioned this pull request Nov 27, 2024

DQ fix rope creation #12073

Merged

StekPerepolnen mentioned this pull request Nov 29, 2024

viewer script execute swagger #12137

Merged

MBkkt mentioned this pull request Jan 16, 2025

👑 Global vector index #8967

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: vector index description wasn't persisted #11969

Fix: vector index description wasn't persisted #11969

MBkkt commented Nov 25, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

ijon Nov 26, 2024 •

edited

Loading

MBkkt Nov 26, 2024

MBkkt Nov 26, 2024

ijon Nov 26, 2024

MBkkt Nov 26, 2024 •

edited

Loading

ijon Nov 26, 2024

MBkkt Nov 26, 2024 •

edited

Loading

ijon Nov 26, 2024

MBkkt Nov 26, 2024

ijon Nov 26, 2024

ijon Nov 26, 2024

MBkkt Nov 26, 2024

ijon Nov 26, 2024

MBkkt Nov 26, 2024

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

Fix: vector index description wasn't persisted #11969

Fix: vector index description wasn't persisted #11969

Conversation

MBkkt commented Nov 25, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Nov 25, 2024 • edited Loading

github-actions bot commented Nov 25, 2024 • edited Loading

ijon Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBkkt Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024 • edited Loading

github-actions bot commented Nov 26, 2024 • edited Loading

github-actions bot commented Nov 26, 2024 • edited Loading

github-actions bot commented Nov 26, 2024 • edited Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

ijon Nov 26, 2024 •

edited

Loading

MBkkt Nov 26, 2024 •

edited

Loading

MBkkt Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading