github-actions[bot] commented on code in PR #65076:
URL: https://github.com/apache/doris/pull/65076#discussion_r3585072943


##########
be/test/olap/rowset/group_rowset_writer_test.cpp:
##########
@@ -225,4 +261,89 @@ TEST_F(GroupRowsetWriterTest, success) {
     EXPECT_TRUE(file_exists(second_segment_path));
 }
 
+TEST_F(GroupRowsetWriterTest, partialUpdateSkipsHiddenNonKeyColumns) {
+    ASSERT_EQ(1, 
_tablet->row_binlog_tablet_schema()->field_index("__DORIS_TEST_HIDDEN_KEY__"));
+    ASSERT_EQ(-1, 
_tablet->row_binlog_tablet_schema()->field_index("__DORIS_TEST_HIDDEN_VALUE__"));
+    ASSERT_EQ(-1, 
_tablet->row_binlog_tablet_schema()->field_index(DELETE_SIGN));
+
+    auto partial_update_info = std::make_shared<PartialUpdateInfo>();
+    ASSERT_TRUE(
+            partial_update_info
+                    ->init(_tablet->tablet_id(), 1, *_tablet->tablet_schema(),
+                           UniqueKeyUpdateModePB::UPDATE_FIXED_COLUMNS,
+                           PartialUpdateNewRowPolicyPB::APPEND,
+                           {"k1", "__DORIS_TEST_HIDDEN_KEY__", 
"__DORIS_TEST_HIDDEN_VALUE__", "v1"},
+                           false, 0, 0, "", "")
+                    .ok());
+    EXPECT_EQ((std::vector<uint32_t> {0, 1, 2, 3}), 
partial_update_info->update_cids);
+    EXPECT_EQ((std::vector<uint32_t> {4, 5}), 
partial_update_info->missing_cids);
+
+    RowsetWriterContext row_binlog_context;
+    row_binlog_context.tablet = _tablet;
+    row_binlog_context.tablet_schema = _tablet->row_binlog_tablet_schema();
+    row_binlog_context.rowset_state = PREPARED;
+    row_binlog_context.segments_overlap = NONOVERLAPPING;

Review Comment:
   This does not actually cover the existing-row path where the hidden key 
mapping can break. The `MowContext` here has an empty `rowset_ptrs`, and 
`retrieve_historical_row()` only probes those rowsets, so this test takes the 
`KEY_NOT_FOUND` branch and records an append/default row; the `EXPECT_TRUE(... 
column 3 ... is_null_at(0))` assertion confirms the missing value came from the 
new-row/default path. Since the risky change is using hidden key columns to 
locate and fill historical rows, please seed an existing rowset, pass it 
through `MowContext`, then assert the partial update produces the matched-row 
result (missing AFTER value and BEFORE image populated, with update/delete op 
as appropriate).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to