github-actions[bot] commented on code in PR #65826:
URL: https://github.com/apache/doris/pull/65826#discussion_r3635723411
##########
be/src/storage/iterator/block_reader.cpp:
##########
@@ -381,7 +384,7 @@ Status BlockReader::_detail_change_next_block(Block* block,
bool* eof) {
output_row_count++;
} else if (op == ROW_BINLOG_DELETE) {
Review Comment:
DETAIL is also supported for the compatibility layout where the row-binlog
schema still contains `__BEFORE__*` columns but `write_before` is false.
`RowBinlogSegmentWriter` marks that layout with `_fill_empty_before_value` and
deliberately stores NULL placeholders in every BEFORE column. With
`use_before=true`, `_resolve_source_column_index` sees that the mirror exists
and emits the placeholder instead of the stored DELETE payload (for example, a
full-payload delete becomes NULL). Please preserve the current column when
history was not recorded, or carry enough metadata to distinguish placeholder
mirrors, and add a DETAIL DELETE case for this layout.
##########
be/src/storage/iterator/block_reader.cpp:
##########
@@ -290,9 +296,6 @@ Status BlockReader::_min_delta_next_block(Block* block,
bool* eof) {
if (idx == _binlog_op_pos) {
RETURN_IF_ERROR(_write_binlog_op(*target_columns[target_col_idx],
Review Comment:
This also removes the existing `_binlog_lsn_pos` special case, although the
bug being fixed only requires payload columns to use the first BEFORE image.
For updates to key 1 at LSN 100 and 102 with another key's event at 101, the
folded pair now carries LSNs 100/102; `ORDER BY __DORIS_BINLOG_LSN__`
interleaves the unrelated event between UPDATE_BEFORE/UPDATE_AFTER. Previously
both halves used the group's last LSN, consistent with the write-side max-LSN
merge contract. Please restore last-row selection for LSN and add an assertion
for the multi-update pair.
##########
regression-test/suites/table_stream_p0/test_mow_min_delta_delete_before.groovy:
##########
@@ -0,0 +1,125 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_mow_min_delta_delete_before", "nonConcurrent") {
+ if (isCloudMode()) {
+ return
+ }
+ sql "DROP DATABASE IF EXISTS test_mow_min_delta_delete_before_db"
+ sql "CREATE DATABASE test_mow_min_delta_delete_before_db"
+ sql "USE test_mow_min_delta_delete_before_db"
+ sql "set enable_nereids_planner=true"
+ sql "set enable_fallback_to_original_planner=false"
+
+ try {
+ sql "DROP STREAM IF EXISTS mow_min_delta_bug_stream"
+ sql "DROP TABLE IF EXISTS mow_min_delta_bug"
+
+ // MoW UNIQUE KEY + MIN_DELTA: when an UPDATE and a DELETE on the same
key
+ // are folded into a single DELETE, the emitted DELETE row must carry
the
+ // pre-delete snapshot (the UPDATE_BEFORE old value), not the
UPDATE_AFTER
+ // new value nor the DELETE tombstone value.
+ sql """
+ CREATE TABLE mow_min_delta_bug (
+ id BIGINT,
+ v1 INT
+ ) ENGINE=OLAP
+ UNIQUE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "enable_unique_key_merge_on_write" = "true",
+ "binlog.enable" = "true",
+ "binlog.format" = "ROW",
+ "binlog.need_historical_value" = "true"
+ )
+ """
+ // Seed old value before the stream starts so the in-window change is
an
+ // UPDATE (not a fresh APPEND).
+ sql "INSERT INTO mow_min_delta_bug VALUES (1, 10)"
+ sql """
+ CREATE STREAM mow_min_delta_bug_stream
+ ON TABLE mow_min_delta_bug
+ PROPERTIES (
+ "type" = "min_delta",
+ "show_initial_rows" = "false"
+ )
+ """
+ sql "sync"
+ sql "INSERT INTO mow_min_delta_bug VALUES (1, 11)"
+ sql "DELETE FROM mow_min_delta_bug WHERE id = 1"
+ sql "sync"
+ sleep(1200)
+
+ // Expect a single DELETE row keeping the old value 10.
+ order_qt_mow_min_delta_delete_before """
+ SELECT id, v1, __DORIS_STREAM_CHANGE_TYPE_COL__
+ FROM mow_min_delta_bug_stream
+ ORDER BY id, v1, __DORIS_STREAM_CHANGE_TYPE_COL__
+ """
+
+ sql "DROP STREAM IF EXISTS mow_min_delta_del_ins_del_stream"
+ sql "DROP TABLE IF EXISTS mow_min_delta_del_ins_del"
+
+ // MoW UNIQUE KEY + MIN_DELTA: when a DELETE, a re-INSERT and another
DELETE
+ // on the same key are folded into a single DELETE, the first op is a
DELETE,
+ // which means the key already existed before the window. The emitted
DELETE
+ // row must carry the first op's pre-delete snapshot (the original
value 20),
+ // not the re-inserted value 21.
+ sql """
+ CREATE TABLE mow_min_delta_del_ins_del (
+ id BIGINT,
+ v1 INT
+ ) ENGINE=OLAP
+ UNIQUE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "enable_unique_key_merge_on_write" = "true",
+ "binlog.enable" = "true",
+ "binlog.format" = "ROW",
+ "binlog.need_historical_value" = "true"
+ )
+ """
+ // Seed old value before the stream starts so the first in-window
DELETE
+ // refers to a pre-existing row.
+ sql "INSERT INTO mow_min_delta_del_ins_del VALUES (2, 20)"
+ sql """
+ CREATE STREAM mow_min_delta_del_ins_del_stream
+ ON TABLE mow_min_delta_del_ins_del
+ PROPERTIES (
+ "type" = "min_delta",
+ "show_initial_rows" = "false"
+ )
+ """
+ sql "sync"
+ sql "DELETE FROM mow_min_delta_del_ins_del WHERE id = 2"
+ sql "INSERT INTO mow_min_delta_del_ins_del VALUES (2, 21)"
+ sql "DELETE FROM mow_min_delta_del_ins_del WHERE id = 2"
+ sql "sync"
+ sleep(1200)
+
+ // Expect a single DELETE row keeping the old value 20.
+ order_qt_mow_min_delta_del_ins_del """
+ SELECT id, v1, __DORIS_STREAM_CHANGE_TYPE_COL__
+ FROM mow_min_delta_del_ins_del_stream
+ ORDER BY id, v1, __DORIS_STREAM_CHANGE_TYPE_COL__
+ """
+ } finally {
Review Comment:
Please do not drop the database in `finally`. The suite already
drops/recreates it before use, and the repository test standard intentionally
keeps tables after a failure so the generated stream/binlog state can be
inspected. Removing this final cleanup (and the now-unneeded `try/finally`)
preserves that debugging state for either `order_qt` failure.
--
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]
