Use the whole TTableDescription to describe an indexImplTable in TIndexDescription #6280

jepett0 · 2024-07-04T08:51:18Z

It would be better (more future proof) to keep all the necessary settings of the indexImplTable in one message. I would not like to reinvent the bicycle, so the best option I see here is to reuse TTableDescription.

Additionally, I have added options fillConfig and fillBoundaries to control what parts of the whole TTableDescription would be return on DescribeTableIndex request. It should prevent the unnecessary use of CPU for parsing big protobufs (TableBoundaries can be big). We would need to enable fillBoundaries explicitly for backups, but this would be done in scope of #4955.

And the last change is to make IndexImplTableDescription field a repeated field. The motivation is to prepare flat_scheme_op.proto for multiple indexImplTables which are said to be coming soon to implement vector indices. It is a breaking change, but there are no instances of YDB that have been using the new TIndexDescriptions with PartitionConfig and PartitionBoundaries, so it should be ok.

jepett0 · 2024-07-04T09:06:53Z

ydb/core/grpc_services/rpc_describe_table.cpp

@@ -92,7 +92,14 @@ class TDescribeTableRPC : public TRpcSchemeRequestActor<TDescribeTableRPC, TEvDe
                    return Reply(Ydb::StatusIds::INTERNAL_ERROR, ctx);
                }

-                FillIndexDescription(describeTableResult, tableDescription, splitKeyType);
+                try {
+                    FillIndexDescription(describeTableResult, tableDescription);


FillIndexDescription now calls FillColumnDescription which might throw

jepett0 · 2024-07-04T09:13:20Z

ydb/core/tx/schemeshard/schemeshard_path_describer.cpp

-    ) {
-        *entry.MutableExplicitPartitions()->MutableSplitBoundary() = explicitPartitions;
-    }
+    *entry.AddIndexImplTableDescription() = indexImplTable->TableDescription;


The entire TableDescription is copied here in the TIndexDescription. However, it does not contain a lot of info. Mainly, it does not contain TTablePartition which is heavy for multishard tables.

I wonder why there is no column info in the IndexImplTableDescription... I will solve this problem in scope of #4955

jepett0 · 2024-07-04T09:14:46Z

ydb/core/ydb_convert/table_description.cpp

-    const NKikimrMiniKQL::TType& splitKeyType
-) {
-    for (const auto& boundary : boundaries) {
+        const NKikimrSchemeOp::TTableDescription& in, const NKikimrMiniKQL::TType& splitKeyType) {


These changes restore these lines to the original state in which they were before the #5119

jepett0 · 2024-07-04T09:18:46Z

ydb/core/ydb_convert/table_description.cpp

+    if (indexImplTableDescription.HasUniformPartitionsCount()) {
+        settings.set_uniform_partitions(indexImplTableDescription.GetUniformPartitionsCount());
+    }
+    if (indexImplTableDescription.SplitBoundarySize()) {


You could ask yourself: "What if BOTH UniformPartitionsCount and SplitBoundary are specified?" My answer is: SplitBoundary has higher priority. This method (FillIndexDescription) prepares a description of an index. It does not create the index! So SplitBoundary is more important in this scenario.

The other option I see here is to throw an error. (FillIndexDescription already throws if FillColumnDescription cannot decide how to fill the columns.)

The other option I see here is to throw an error. (FillIndexDescription already throws if FillColumnDescription cannot decide how to fill the columns.)

Exception sounds better for me, because

looks like error case

can be changed to something else in future

I don't think having two different behavior in such case is an good idea

I checked how DescribeTable deals with this problem and found out that it just ignores UniformPartitionsCount, i.e. it does not even check if it exists in the TTableDescription. See the code around here. The motivation is: there is no such thing as UniformPartitionsCount for an existing table. If a table already exists, then it has explicit partition boundaries which are represented by SplitBoundary field of the TTableDescription protobuf.

azevaykin · 2024-07-04T09:34:23Z

ydb/core/protos/flat_scheme_op.proto

-        TExplicitPartitions ExplicitPartitions = 11;
-    }
-    optional TPartitioningPolicy PartitioningPolicy = 12;
+    repeated TTableDescription IndexImplTableDescription = 10;


According to the Language Guide (proto 3) , repeated fields are used in the plural.

Suggested change

repeated TTableDescription IndexImplTableDescription = 10;

repeated TTableDescription IndexImplTableDescriptions = 10;

Ok. I agree. More on the issue:

https://protobuf.dev/programming-guides/style/#repeated-fields :

Use pluralized names for repeated fields.

https://google.aip.dev/144 :

Repeated fields must use a plural field name.

statistically speaking, only 12 out of 94 repeated protobuf field names are singular in flat_scheme_op.proto

MBkkt · 2024-07-04T09:48:19Z

ydb/core/protos/flat_scheme_op.proto

@@ -1000,15 +996,10 @@ message TIndexDescription {
    optional uint64 PathOwnerId = 7;

    repeated string DataColumnNames = 8;
+
    // DataSize + IndexSize of indexImplTable


Suggested change

// DataSize + IndexSize of indexImplTable

// DataSize + IndexSize

What is IndexSize? Is it size of index or size of all (now single) index impl tables?

IndexSize is the size of the index (the internal index that implements the primary key logic) of the indexImplTable. It follows the general DataSize + IndexSize = TotalSize convention for tables.

I would much rather leave this comment for you to deal with in scope of allowing multiple indexImplTables 😜 My idea for this PR is to create a hotfix before it is too late, not to correct every assumption in the code that the indexImplTable is single

ydb/core/tx/datashard/export_common.cpp

MBkkt · 2024-07-04T11:15:39Z

ydb/core/ydb_convert/table_description.cpp

+    if (indexImplTableDescription.HasUniformPartitionsCount()) {
+        settings.set_uniform_partitions(indexImplTableDescription.GetUniformPartitionsCount());
+    }
+    if (indexImplTableDescription.SplitBoundarySize()) {


The other option I see here is to throw an error. (FillIndexDescription already throws if FillColumnDescription cannot decide how to fill the columns.)

Exception sounds better for me, because

looks like error case

can be changed to something else in future

I don't think having two different behavior in such case is an good idea

jepett0 · 2024-07-04T15:41:58Z

ydb/core/tx/schemeshard/schemeshard_impl.h

-    void DescribeTableIndex(const TPathId& pathId, const TString& name, NKikimrSchemeOp::TIndexDescription& entry);
-    void DescribeTableIndex(const TPathId& pathId, const TString& name, TTableIndexInfo::TPtr indexInfo, NKikimrSchemeOp::TIndexDescription& entry);
+    void DescribeTableIndex(const TPathId& pathId, const TString& name,
+        bool fillConfig, bool fillBoundaries, NKikimrSchemeOp::TIndexDescription& entry


I have added explicit fillConfig and fillBoundaries options to be able to control the behavior of DescribeTableIndex. Moreover, these options are supported by DescribeTable and it would be inconsistent to ignore them in DescribeTableIndex

jepett0 · 2024-07-04T15:42:57Z

ydb/core/tx/schemeshard/schemeshard_path_describer.cpp

+void FillPartitionConfig(const NKikimrSchemeOp::TPartitionConfig& in, NKikimrSchemeOp::TPartitionConfig& out) {
+    out.CopyFrom(in);
+    NKikimr::NSchemeShard::TPartitionConfigMerger::DeduplicateColumnFamiliesById(out);
+    out.MutableStorageRooms()->Clear();


It turns out that PartitionConfig is not just copied, but some unnecessary info is erased before giving it out to users. I have reused this logic for both DescribeTable and DescribeTableIndex.

github-actions · 2024-07-05T11:09:34Z

⚪ 2024-07-05 11:09:34 UTC Pre-commit check for ee33351 has started.
⚪ 2024-07-05 11:12:39 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-07-05 11:59:32 UTC Build successful.
⚪ 2024-07-05 11:59:59 UTC Tests are running...
🔴 2024-07-05 13:45:25 UTC Some tests failed, follow the links below.

Test history | Test log

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
39750	34516	0	3	5221	10

🟢 2024-07-05 13:46:06 UTC ydbd size 8.1 GiB changed* by -120 Bytes, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `5fc09ca`	merge: `ee33351`	diff	diff %
ydbd size	8 726 129 816 Bytes	8 726 129 696 Bytes	-120 Bytes	-0.000%
ydbd stripped size	477 144 232 Bytes	477 142 216 Bytes	-2.0 KiB	-0.000%

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

…exDescription (ydb-platform#6280) The purpose of the change is to prepare for multiple indexImplTables for vector indices. Other changes are more or less cosmetic.

…exDescription (#6280) (#6450)

jepett0 requested review from azevaykin and ijon July 4, 2024 09:05

jepett0 commented Jul 4, 2024

View reviewed changes

jepett0 requested a review from MBkkt July 4, 2024 09:23

azevaykin reviewed Jul 4, 2024

View reviewed changes

ydb-platform deleted a comment from github-actions bot Jul 4, 2024

This comment was marked as outdated.

Sign in to view

ydb-platform deleted a comment from github-actions bot Jul 4, 2024

This comment was marked as outdated.

Sign in to view

azevaykin previously approved these changes Jul 4, 2024

View reviewed changes

MBkkt reviewed Jul 4, 2024

View reviewed changes

jepett0 dismissed azevaykin’s stale review via 27f1843 July 4, 2024 14:59

This comment was marked as outdated.

Sign in to view

jepett0 requested review from MBkkt and azevaykin July 4, 2024 15:10

jepett0 marked this pull request as ready for review July 4, 2024 15:39

jepett0 commented Jul 4, 2024

View reviewed changes

azevaykin previously approved these changes Jul 5, 2024

View reviewed changes

MBkkt previously approved these changes Jul 5, 2024

View reviewed changes

jepett0 changed the title ~~Prepare flat_scheme_op.proto for multiple indexImplTables~~ Use the whole TTableDescription to describe indexImplTable in TIndexDescription Jul 5, 2024

jepett0 changed the title ~~Use the whole TTableDescription to describe indexImplTable in TIndexDescription~~ Use the whole TTableDescription to describe an indexImplTable in TIndexDescription Jul 5, 2024

jepett0 dismissed MBkkt’s stale review via cd8f8c5 July 5, 2024 09:50

jepett0 requested review from MBkkt and azevaykin July 5, 2024 11:42

azevaykin approved these changes Jul 5, 2024

View reviewed changes

jepett0 enabled auto-merge (squash) July 5, 2024 12:03

jepett0 disabled auto-merge July 5, 2024 12:52

jepett0 merged commit fb31079 into ydb-platform:main Jul 5, 2024
8 of 12 checks passed

GrigoriyPA mentioned this pull request Jul 8, 2024

YQ-3367 frozen fq yt-kqp test list #6397

Merged

jepett0 self-assigned this Jul 8, 2024

jepett0 mentioned this pull request Jul 9, 2024

Use the whole TTableDescription to describe an indexImplTable in TIndexDescription (#6280) #6450

Merged

This was referenced Jul 9, 2024

Kqprun added request time info printing #6479

Merged

YQ-3405 fixed endless retries for external error #6626

Merged

YQ-3345 fixed WM counters and unit test #6643

Merged

jepett0 added a commit that referenced this pull request Jul 16, 2024

Use the whole TTableDescription to describe an indexImplTable in TInd…

14c79b8

…exDescription (#6280) (#6450)

This was referenced Jul 24, 2024

YQ WM fix for sls feature flag #7057

Merged

YQ WM fixed build errors #7127

Closed

niksaveliev mentioned this pull request Aug 15, 2024

New kafka metering fix #7856

Closed

This was referenced Aug 16, 2024

Fix use after move in http_proxy #7884

Closed

fix use after move in http_proxy #7885

Merged

StekPerepolnen mentioned this pull request Aug 23, 2024

merge/stable 24 3/query float test asan #8192

Closed

siarheivesialou mentioned this pull request Aug 28, 2024

stable 24 3 6 logbroker add 5 last requests #8389

Closed

swalrus1 mentioned this pull request Sep 3, 2024

Update stable-24-3: add metrics to describe table #8688

Closed

This was referenced Oct 29, 2024

viewer healthcheck pathmerge stable 24 3 11 hotfix #11042

Closed

viewer healthcheck pathmerge stable 24 #11044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the whole TTableDescription to describe an indexImplTable in TIndexDescription #6280

Use the whole TTableDescription to describe an indexImplTable in TIndexDescription #6280

jepett0 commented Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024

jepett0 Jul 4, 2024

jepett0 Jul 4, 2024

jepett0 Jul 4, 2024

jepett0 Jul 4, 2024 •

edited

Loading

MBkkt Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024

azevaykin Jul 4, 2024

jepett0 Jul 4, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

MBkkt Jul 4, 2024

jepett0 Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

MBkkt Jul 4, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jepett0 Jul 4, 2024

jepett0 Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading

	repeated TTableDescription IndexImplTableDescription = 10;
	repeated TTableDescription IndexImplTableDescriptions = 10;

	// DataSize + IndexSize of indexImplTable
	// DataSize + IndexSize

Use the whole TTableDescription to describe an indexImplTable in TIndexDescription #6280

Use the whole TTableDescription to describe an indexImplTable in TIndexDescription #6280

Conversation

jepett0 commented Jul 4, 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

jepett0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

MBkkt Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jepett0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

jepett0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

jepett0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

MBkkt Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

jepett0 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 5, 2024 • edited Loading

jepett0 commented Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

MBkkt Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

MBkkt Jul 4, 2024 •

edited

Loading

jepett0 Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading