KIKIMR-5850: tablet cleanup and extra comments

ref:c9b3417f75ca839ff0e0cff6563992a197ef12cb

17 files changed, 63 insertions, 40 deletions

diff --git a/ydb/core/tablet_flat/flat_executor_ut.cpp b/ydb/core/tablet_flat/flat_executor_ut.cpp
index 6eafc0ad960..419167facf2 100644
--- a/ydb/core/tablet_flat/flat_executor_ut.cpp
+++ b/ydb/core/tablet_flat/flat_executor_ut.cpp
@@ -2724,7 +2724,7 @@ Y_UNIT_TEST_SUITE(TFlatTableExecutorKeepEraseMarkers) {
                 DbgPrintValue(value, row.Get(0), NScheme::TString::TypeId);
 
                 builder << "Key " << keyId << " = " << row.GetRowState()
-                    << " value = " << NTable::ECellOp(row.GetOp(0)) << " " << value << Endl;
+                    << " value = " << NTable::ECellOp(row.GetCellOp(0)) << " " << value << Endl;
             }
 
             Data = builder;
@@ -2985,7 +2985,7 @@ Y_UNIT_TEST_SUITE(TFlatTableExecutorMoveTableData) {
                 DbgPrintValue(value, row.Get(1), NScheme::TString::TypeId);
 
                 builder << "Key " << key << " = " << row.GetRowState()
-                    << " value = " << NTable::ECellOp(row.GetOp(1)) << " " << value << Endl;
+                    << " value = " << NTable::ECellOp(row.GetCellOp(1)) << " " << value << Endl;
             }
 
             Data = builder;
@@ -3298,7 +3298,7 @@ Y_UNIT_TEST_SUITE(TFlatTableExecutorFollower) {
                 DbgPrintValue(value, row.Get(1), NScheme::TString::TypeId);
 
                 builder << "Key " << key << " = " << row.GetRowState()
-                    << " value = " << NTable::ECellOp(row.GetOp(1)) << " " << value << Endl;
+                    << " value = " << NTable::ECellOp(row.GetCellOp(1)) << " " << value << Endl;
             }
 
             Data = builder;
@@ -4078,8 +4078,8 @@ Y_UNIT_TEST_SUITE(TFlatTableLongTx) {
                 DbgPrintValue(value2, row.Get(2), NScheme::TString::TypeId);
 
                 builder << "Key " << key << " = " << row.GetRowState()
-                    << " value = " << NTable::ECellOp(row.GetOp(1)) << " " << value
-                    << " value2 = " << NTable::ECellOp(row.GetOp(2)) << " " << value2 << Endl;
+                    << " value = " << NTable::ECellOp(row.GetCellOp(1)) << " " << value
+                    << " value2 = " << NTable::ECellOp(row.GetCellOp(2)) << " " << value2 << Endl;
             }
 
             Data = builder;
@@ -4127,8 +4127,8 @@ Y_UNIT_TEST_SUITE(TFlatTableLongTx) {
                     DbgPrintValue(value2, row.Get(2), NScheme::TString::TypeId);
 
                     builder << "Key " << key << " = " << row.GetRowState()
-                        << " value = " << NTable::ECellOp(row.GetOp(1)) << " " << value
-                        << " value2 = " << NTable::ECellOp(row.GetOp(2)) << " " << value2;
+                        << " value = " << NTable::ECellOp(row.GetCellOp(1)) << " " << value
+                        << " value2 = " << NTable::ECellOp(row.GetCellOp(2)) << " " << value2;
                     if (it->IsUncommitted()) {
                         builder << " txId " << it->GetUncommittedTxId();
                     }
diff --git a/ydb/core/tablet_flat/flat_executor_ut_large.cpp b/ydb/core/tablet_flat/flat_executor_ut_large.cpp
index 6bd3cc712ba..0a163e6bc62 100644
--- a/ydb/core/tablet_flat/flat_executor_ut_large.cpp
+++ b/ydb/core/tablet_flat/flat_executor_ut_large.cpp
@@ -131,8 +131,8 @@ Y_UNIT_TEST_SUITE(TFlatTableLongTxLarge) {
                 DbgPrintValue(value2, row.Get(2), NScheme::TString::TypeId);
 
                 builder << "Key " << key << " = " << row.GetRowState()
-                    << " value = " << NTable::ECellOp(row.GetOp(1)) << " " << value
-                    << " value2 = " << NTable::ECellOp(row.GetOp(2)) << " " << value2 << Endl;
+                    << " value = " << NTable::ECellOp(row.GetCellOp(1)) << " " << value
+                    << " value2 = " << NTable::ECellOp(row.GetCellOp(2)) << " " << value2 << Endl;
             }
 
             Data = builder;
diff --git a/ydb/core/tablet_flat/flat_mem_iter.h b/ydb/core/tablet_flat/flat_mem_iter.h
index 55223cabe21..8eb790fcbdc 100644
--- a/ydb/core/tablet_flat/flat_mem_iter.h
+++ b/ydb/core/tablet_flat/flat_mem_iter.h
@@ -310,7 +310,7 @@ namespace NTable {
         void ApplyColumn(TRowState& row, const NMem::TColumnUpdate &up) const noexcept
         {
             const auto pos = Remap->Has(up.Tag);
-            auto op = TCellOp::By(up.Op);
+            auto op = TCellOp::Decode(up.Op);
 
             if (!pos || row.IsFinalized(pos)) {
                 /* Out of remap or row slot is already filled */
diff --git a/ydb/core/tablet_flat/flat_page_base.h b/ydb/core/tablet_flat/flat_page_base.h
index 48671596d42..69d2b8ff7fc 100644
--- a/ydb/core/tablet_flat/flat_page_base.h
+++ b/ydb/core/tablet_flat/flat_page_base.h
@@ -173,8 +173,17 @@ struct TRecordsEntry {
     TPgSize Offset;
 } Y_PACKED;
 
+// The idea is that every page record consists from its header defined in TRecord
+// and TItems each located at its own TPartScheme::TColumn::Offset.
+// TItem::GetCellOp specifies TCell location/value:
+//   * empty/null/reset (just CellOp code, no actual bytes)
+//   * fixed size right after TItem
+//   * variable size at some page position, TDataRef is after TItem and contains position and size
+//   * in external blob
 template <class TRecord, class TItem>
 struct TDataPageRecord {
+    // TRecord acts as record's header holder, Base() skips header
+    // and returns pointer to the beginning of data where items stored
     void* Base() {
         return NextPtr((TRecord*)this);
     }
@@ -192,11 +201,6 @@ struct TDataPageRecord {
     }
 
     template <class T>
-    const T* GetTail(const TPartScheme::TGroupInfo& group) const {
-        return TDeref<T>::At(Base(), group.FixedSize);
-    }
-
-    template <class T>
     T* GetFixed(TItem* item) const {
         return reinterpret_cast<T*>(NextPtr(item));
     }
@@ -206,15 +210,15 @@ struct TDataPageRecord {
         return reinterpret_cast<const T*>(NextPtr(item));
     }
 
-    TCellOp GetOp(const TPartScheme::TColumn &info) const
+    TCellOp GetCellOp(const TPartScheme::TColumn &info) const
     {
-        return GetItem(info)->GetOp(info.IsKey());
+        return GetItem(info)->GetCellOp(info.IsKey());
     }
 
     TCell Cell(const TPartScheme::TColumn& info) const
     {
         const TItem* item = GetItem(info);
-        const auto op = item->GetOp(info.IsKey());
+        const auto op = item->GetCellOp(info.IsKey());
 
         if (op == ECellOp::Empty || op == ECellOp::Null || op == ECellOp::Reset) {
             return { };
diff --git a/ydb/core/tablet_flat/flat_page_data.h b/ydb/core/tablet_flat/flat_page_data.h
index 6e62fc2101d..df083d6b4a8 100644
--- a/ydb/core/tablet_flat/flat_page_data.h
+++ b/ydb/core/tablet_flat/flat_page_data.h
@@ -21,7 +21,7 @@ namespace NPage {
         /*
                 TRecord binary layout v1
             .-------------------------.
-            | ERowOp                  | header
+            | ERowOp and bit-flags    | header
             .---------.---------------.       -.
             | cell op | value OR offs | cell_1 |
             .---------.---------------.        |
@@ -36,10 +36,11 @@ namespace NPage {
 #pragma pack(push,1)
 
         struct TItem {
-            TCellOp GetOp(bool key) const noexcept
+            TCellOp GetCellOp(bool key) const noexcept
             {
-                return
-                    key ? TCellOp(Null ? ECellOp::Null : ECellOp::Set) : TCellOp::By(Flg);
+                if (key)
+                    return Null ? ECellOp::Null : ECellOp::Set;
+                return TCellOp::Decode(Flg);
             }
 
             union {
@@ -110,6 +111,9 @@ namespace NPage {
                 Fields_ |= 0x20;
             }
 
+            // N.B. Get Min/Max version and GetDeltaTxId below read from the same record position
+            // right after column entries. Up to caller to decide what kind of data is there
+
             TRowVersion GetMaxVersion(const TPartScheme::TGroupInfo& group) const noexcept {
                 Y_VERIFY_DEBUG(IsErased());
                 return GetTail<TVersion>(group)->Get();
@@ -141,6 +145,13 @@ namespace NPage {
                     return nullptr;
                 }
             }
+
+        private:
+            // returns pointer to the data right after all column entries
+            template <class T>
+            const T* GetTail(const TPartScheme::TGroupInfo& group) const {
+                return TDeref<T>::At(Base(), group.FixedSize);
+            }
         } Y_PACKED;
 
         struct TExtra {
diff --git a/ydb/core/tablet_flat/flat_page_index.h b/ydb/core/tablet_flat/flat_page_index.h
index 712ebf06de8..c0b50a5843c 100644
--- a/ydb/core/tablet_flat/flat_page_index.h
+++ b/ydb/core/tablet_flat/flat_page_index.h
@@ -28,9 +28,11 @@ namespace NPage {
 #pragma pack(push,1)
 
         struct TItem {
-            TCellOp GetOp(bool) const noexcept
+            TCellOp GetCellOp(bool) const noexcept
             {
-                return { Null ? ECellOp::Null : ECellOp::Set, ELargeObj::Inline };
+                if (Null)
+                    return TCellOp(ECellOp::Null);
+                return TCellOp(ECellOp::Set, ELargeObj::Inline);
             }
 
             bool Null;
diff --git a/ydb/core/tablet_flat/flat_page_label.cpp b/ydb/core/tablet_flat/flat_page_label.cpp
index 78586bda02b..894722e1f3d 100644
--- a/ydb/core/tablet_flat/flat_page_label.cpp
+++ b/ydb/core/tablet_flat/flat_page_label.cpp
@@ -40,7 +40,9 @@ namespace NPage {
                 in-place crc (may be) or other pages common metadata.
              */
 
+            // this ugly construct is just to get ui8 at raw.begin() + sizeof(TLabel)
             auto codec = ECodec(*TDeref<ui8>::At(TDeref<TLabel>::At(raw.begin(), 0) + 1, 0));
+
             auto *on = raw.begin() + sizeof(TLabel) + 8;
 
             return { label.Type, version, codec, { on, raw.end() } };
diff --git a/ydb/core/tablet_flat/flat_page_label.h b/ydb/core/tablet_flat/flat_page_label.h
index ef14f4bda47..ba0d1df80e7 100644
--- a/ydb/core/tablet_flat/flat_page_label.h
+++ b/ydb/core/tablet_flat/flat_page_label.h
@@ -12,9 +12,13 @@ namespace NKikimr {
 namespace NTable {
 namespace NPage {
 
+    // N.B. this struct includes only fields from old style label,
+    // new style label (most significant bit is set is Format) contains
+    // extra 8 bytes, first one is codec, the rest is reserved.
+    // See TLabelWrapper::Read()
     struct TLabel {
         EPage Type;
-        ui16 Format;
+        ui16 Format; // most significant bit is format indicator, the rest is version
         TSize Size;
 
         void Init(EPage type, ui16 format, ui64 size) noexcept;
diff --git a/ydb/core/tablet_flat/flat_page_writer.h b/ydb/core/tablet_flat/flat_page_writer.h
index 005d3dd54c9..8c931c12be7 100644
--- a/ydb/core/tablet_flat/flat_page_writer.h
+++ b/ydb/core/tablet_flat/flat_page_writer.h
@@ -357,7 +357,7 @@ namespace NPage {
 
                 if (!row.IsFinalized(pin.To) || info.IsKey() || info.IsFixed) {
 
-                } else if (row.GetOp(pin.To) != ELargeObj::Inline) {
+                } else if (row.GetCellOp(pin.To) != ELargeObj::Inline) {
                     /* External blob occupies only fixed technical field */
                     ++ret.ReusedLargeRefs;
                 } else if (!isDelta && IsLargeSize(row.Get(pin.To).Size())) {
@@ -366,7 +366,7 @@ namespace NPage {
                 } else if (!isDelta && IsSmallSize(row.Get(pin.To).Size())) {
                     ret.SmallSize += row.Get(pin.To).Size();
                     ++ret.NewSmallRefs;
-                } else if (isDelta || !finalRow || row.GetOp(pin.To) != ECellOp::Reset) {
+                } else if (isDelta || !finalRow || row.GetCellOp(pin.To) != ECellOp::Reset) {
                     ret.DataPageSize += row.Get(pin.To).Size();
                 }
             }
@@ -484,11 +484,11 @@ namespace NPage {
 
                 if (!row.IsFinalized(pin.To) || info.IsKey()) {
 
-                } else if (row.GetOp(pin.To) == ECellOp::Reset) {
+                } else if (row.GetCellOp(pin.To) == ECellOp::Reset) {
                     if (!finalRow)
                         rec.GetItem(info)->Flg = ui8(ECellOp::Reset);
                 } else if (auto cell = row.Get(pin.To)) {
-                    auto cellOp = row.GetOp(pin.To);
+                    auto cellOp = row.GetCellOp(pin.To);
 
                     if (info.IsFixed /* may place only as ELargeObj::Inline */) {
                         Y_VERIFY(cellOp == ELargeObj::Inline, "Got fixed non-inlined");
diff --git a/ydb/core/tablet_flat/flat_part_dump.cpp b/ydb/core/tablet_flat/flat_part_dump.cpp
index b10f2865e8d..f78a07e8dfd 100644
--- a/ydb/core/tablet_flat/flat_part_dump.cpp
+++ b/ydb/core/tablet_flat/flat_part_dump.cpp
@@ -173,7 +173,7 @@ namespace {
                 if (info.IsKey())
                     continue;
 
-                const auto op = iter->GetOp(info);
+                const auto op = iter->GetCellOp(info);
 
                 if (op == ECellOp::Empty)
                     continue;
diff --git a/ydb/core/tablet_flat/flat_part_iter_multi.h b/ydb/core/tablet_flat/flat_part_iter_multi.h
index 2b6921854c9..0fb46c3040a 100644
--- a/ydb/core/tablet_flat/flat_part_iter_multi.h
+++ b/ydb/core/tablet_flat/flat_part_iter_multi.h
@@ -1127,7 +1127,7 @@ namespace NTable {
                 const NPage::TDataPage::TRecord* data,
                 const TPartScheme::TColumn& info) const noexcept
         {
-            auto op = data->GetOp(info);
+            auto op = data->GetCellOp(info);
 
             if (op == ECellOp::Empty) {
                 Y_VERIFY(!info.IsKey(), "Got an absent key cell");
diff --git a/ydb/core/tablet_flat/flat_row_eggs.h b/ydb/core/tablet_flat/flat_row_eggs.h
index aaf9e7f35c1..c43f90b8c50 100644
--- a/ydb/core/tablet_flat/flat_row_eggs.h
+++ b/ydb/core/tablet_flat/flat_row_eggs.h
@@ -76,7 +76,7 @@ namespace NTable {
     struct TCellOp {
         constexpr TCellOp() = default;
 
-        static constexpr TCellOp By(ui8 raw) noexcept
+        static constexpr TCellOp Decode(ui8 raw) noexcept
         {
             return { ECellOp(raw & 0x3f), ELargeObj(raw >> 6) };
         }
diff --git a/ydb/core/tablet_flat/flat_row_state.h b/ydb/core/tablet_flat/flat_row_state.h
index a1fda38c260..1bf33250d47 100644
--- a/ydb/core/tablet_flat/flat_row_state.h
+++ b/ydb/core/tablet_flat/flat_row_state.h
@@ -130,7 +130,7 @@ namespace NTable {
             return Cells[pos];
         }
 
-        TCellOp GetOp(TPos pos) const noexcept
+        TCellOp GetCellOp(TPos pos) const noexcept
         {
             return State[pos];
         }
@@ -144,7 +144,7 @@ namespace NTable {
 
         bool IsFinalized(TPos pos) const noexcept
         {
-            return GetOp(pos) != ECellOp::Empty;
+            return GetCellOp(pos) != ECellOp::Empty;
         }
 
     private:
diff --git a/ydb/core/tablet_flat/test/libs/rows/tool.h b/ydb/core/tablet_flat/test/libs/rows/tool.h
index 427d542eaad..18485858665 100644
--- a/ydb/core/tablet_flat/test/libs/rows/tool.h
+++ b/ydb/core/tablet_flat/test/libs/rows/tool.h
@@ -115,13 +115,13 @@ namespace NTest {
 
                 if (keys && !info->IsKey()) {
                     continue; /* Skip non-keys columns */
-                } else if (ELargeObj(val.Op) != ELargeObj(state.GetOp(pos))) {
+                } else if (ELargeObj(val.Op) != ELargeObj(state.GetCellOp(pos))) {
                     return false; /* Missmatched value storage method */
                 } else if (val.Op != ELargeObj::Inline) {
                     if (!(val.Cell.AsRef() == cell.AsRef()))
                         return false;
                 } else if (val.Op == ECellOp::Empty || val.Op == ECellOp::Reset) {
-                    if (ECellOp(val.Op) != state.GetOp(pos))
+                    if (ECellOp(val.Op) != state.GetCellOp(pos))
                         return false;
                 } else if (CompareTypedCells(val.Cell, cell, info->TypeId)) {
                     return false; /* Literal comparison has been failed */
diff --git a/ydb/core/tablet_flat/ut/flat_test_db_helpers.h b/ydb/core/tablet_flat/ut/flat_test_db_helpers.h
index ffc9a36daac..d75e08e3867 100644
--- a/ydb/core/tablet_flat/ut/flat_test_db_helpers.h
+++ b/ydb/core/tablet_flat/ut/flat_test_db_helpers.h
@@ -52,7 +52,7 @@ public:
         return Val;
     }
 
-    ECellOp GetOp() const {
+    ECellOp GetCellOp() const {
         return Op;
     }
 };
@@ -224,7 +224,7 @@ public:
 
         TVector<TUpdateOp> ops;
         for (const auto& op : update.GetTagOps()) {
-            ops.push_back(TUpdateOp(op.first, op.second.GetOp(), op.second.Get()));
+            ops.push_back(TUpdateOp(op.first, op.second.GetCellOp(), op.second.Get()));
         }
 
         Db.Update(update.GetRoot(), ERowOp::Upsert, key, ops);
diff --git a/ydb/core/tx/datashard/change_collector_cdc_stream.cpp b/ydb/core/tx/datashard/change_collector_cdc_stream.cpp
index 4fc67adc6e7..24d533a52a3 100644
--- a/ydb/core/tx/datashard/change_collector_cdc_stream.cpp
+++ b/ydb/core/tx/datashard/change_collector_cdc_stream.cpp
@@ -186,7 +186,7 @@ TRowState TCdcStreamChangeCollector::PatchState(const TRowState& oldState, ERowO
             if (it != updates.end()) {
                 newState.Set(pos, it->second.Op, it->second.AsCell());
             } else if (rop == ERowOp::Upsert) {
-                newState.Set(pos, oldState.GetOp(pos), oldState.Get(pos));
+                newState.Set(pos, oldState.GetCellOp(pos), oldState.Get(pos));
             } else {
                 newState.Set(pos, ECellOp::Null, TCell());
             }
diff --git a/ydb/core/tx/datashard/datashard_common_upload.cpp b/ydb/core/tx/datashard/datashard_common_upload.cpp
index 4daee2067ad..4128209133d 100644
--- a/ydb/core/tx/datashard/datashard_common_upload.cpp
+++ b/ydb/core/tx/datashard/datashard_common_upload.cpp
@@ -156,7 +156,7 @@ bool TCommonUploadOps<TEvRequest, TEvResponse>::Execute(TDataShard* self, TTrans
             }
 
             bool allowUpdate = true;
-            if (readForTableShadow && rowState == NTable::ERowOp::Upsert && rowState.GetOp(vi) != NTable::ECellOp::Empty) {
+            if (readForTableShadow && rowState == NTable::ERowOp::Upsert && rowState.GetCellOp(vi) != NTable::ECellOp::Empty) {
                 // We don't want to overwrite columns that already has some value
                 allowUpdate = false;
             }