Fix null col

Signed-off-by: JaySon-Huang <[email protected]>
pingcap · Sep 15, 2022 · 451ca9f · 451ca9f
1 parent 782269b
commit 451ca9f
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 6 deletions.
diff --git a/dbms/src/Storages/Transaction/RowCodec.cpp b/dbms/src/Storages/Transaction/RowCodec.cpp
@@ -383,7 +383,8 @@ bool appendRowV2ToBlockImpl(
             is_null = idx_null < null_column_ids.size();
 
         auto next_datum_column_id = is_null ? null_column_ids[idx_null] : not_null_column_ids[idx_not_null];
-        if (column_ids_iter->first > next_datum_column_id)
+        const auto next_column_id = column_ids_iter->first;
+        if (next_column_id > next_datum_column_id)
         {
             // The next column id to read is bigger than the column id of next datum in encoded row.
             // It means this is the datum of extra column. May happen when reading after dropping
@@ -396,7 +397,7 @@ bool appendRowV2ToBlockImpl(
             else
                 idx_not_null++;
         }
-        else if (column_ids_iter->first < next_datum_column_id)
+        else if (next_column_id < next_datum_column_id)
         {
             // The next column id to read is less than the column id of next datum in encoded row.
             // It means this is the datum of missing column. May happen when reading after adding
@@ -412,7 +413,7 @@ bool appendRowV2ToBlockImpl(
         {
             // If pk_handle_id is a valid column id, then it means the table's pk_is_handle is true
             // we can just ignore the pk value encoded in value part
-            if (unlikely(column_ids_iter->first == pk_handle_id))
+            if (unlikely(next_column_id == pk_handle_id))
             {
                 column_ids_iter++;
                 block_column_pos++;

diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_block_reader.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_block_reader.cpp
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <Core/Field.h>
 #include <Storages/DeltaMerge/DeltaMergeDefines.h>
 #include <Storages/Transaction/DatumCodec.h>
 #include <Storages/Transaction/DecodingStorageSchemaSnapshot.h>
@@ -75,18 +76,18 @@ class RegionBlockReaderTest : public ::testing::Test
         {
             if (table_info.is_common_handle || table_info.pk_is_handle)
             {
-                if (!table_info.columns[i].hasPriKeyFlag() && !fields[i].isNull())
+                if (!table_info.columns[i].hasPriKeyFlag())
                     value_encode_fields.emplace_back(fields[i]);
                 else
                     key_encode_fields.emplace_back(fields[i]);
             }
-            else if (!fields[i].isNull())
+            else
             {
                 value_encode_fields.emplace_back(fields[i]);
             }
         }
 
-        // create encoded key
+        // create the RawTiDBPK section of encoded key
         WriteBufferFromOwnString pk_buf;
         if (table_info.is_common_handle)
         {
@@ -272,6 +273,8 @@ TEST_F(RegionBlockReaderTest, PKIsNotHandle)
     auto [table_info, fields] = getNormalTableInfoFields({EXTRA_HANDLE_COLUMN_ID}, false);
     ASSERT_EQ(table_info.is_common_handle, false);
     ASSERT_EQ(table_info.pk_is_handle, false);
+    ASSERT_FALSE(table_info.getColumnInfo(2).hasPriKeyFlag());
+
     encodeColumns(table_info, fields, RowEncodeVersion::RowV2);
     auto decoding_schema = getDecodingStorageSchemaSnapshot(table_info);
     ASSERT_TRUE(decodeAndCheckColumns(decoding_schema, true));
@@ -283,6 +286,7 @@ TEST_F(RegionBlockReaderTest, PKIsHandle)
     ASSERT_EQ(table_info.is_common_handle, false);
     ASSERT_EQ(table_info.pk_is_handle, true);
     ASSERT_TRUE(table_info.getColumnInfo(2).hasPriKeyFlag());
+
     encodeColumns(table_info, fields, RowEncodeVersion::RowV2);
     auto decoding_schema = getDecodingStorageSchemaSnapshot(table_info);
     ASSERT_TRUE(decodeAndCheckColumns(decoding_schema, true));
@@ -296,6 +300,7 @@ TEST_F(RegionBlockReaderTest, CommonHandle)
     ASSERT_TRUE(table_info.getColumnInfo(2).hasPriKeyFlag());
     ASSERT_TRUE(table_info.getColumnInfo(3).hasPriKeyFlag());
     ASSERT_TRUE(table_info.getColumnInfo(4).hasPriKeyFlag());
+
     encodeColumns(table_info, fields, RowEncodeVersion::RowV2);
     auto decoding_schema = getDecodingStorageSchemaSnapshot(table_info);
     ASSERT_TRUE(decodeAndCheckColumns(decoding_schema, true));
@@ -366,14 +371,21 @@ TEST_F(RegionBlockReaderTest, OverflowColumnRowV1)
 }
 
 TEST_F(RegionBlockReaderTest, InvalidNULLRowV2)
+try
 {
     auto [table_info, fields] = getNormalTableInfoFields({EXTRA_HANDLE_COLUMN_ID}, false);
+    ASSERT_FALSE(table_info.getColumnInfo(11).hasNotNullFlag()); // col 11 is nullable
+
     encodeColumns(table_info, fields, RowEncodeVersion::RowV2);
+
     auto new_table_info = getTableInfoFieldsForInvalidNULLTest({EXTRA_HANDLE_COLUMN_ID}, false);
+    ASSERT_TRUE(new_table_info.getColumnInfo(11).hasNotNullFlag());  // col 11 is not null
+
     auto new_decoding_schema = getDecodingStorageSchemaSnapshot(new_table_info);
     ASSERT_FALSE(decodeAndCheckColumns(new_decoding_schema, false));
     ASSERT_ANY_THROW(decodeAndCheckColumns(new_decoding_schema, true));
 }
+CATCH
 
 TEST_F(RegionBlockReaderTest, InvalidNULLRowV1)
 {