[fix](delete) fix delete from bug which can get wrong result #17146

nextdreamblue · 2023-02-26T12:43:38Z

Proposed changes

Issue Number: close #17145

Problem summary

Describe your changes.

Checklist(Required)

Does it affect the original behavior
Has unit tests been added
Has document been added or modified
Does it need to update dependencies
Is this PR support rollback (If NO, please explain WHY)

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

github-actions · 2023-02-26T12:47:19Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-02-26T14:47:44Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-02-26T14:58:08Z

clang-tidy review says "All clean, LGTM! 👍"

nextdreamblue · 2023-02-26T15:17:12Z

run buildall

yiguolei · 2023-02-27T01:01:23Z

be/src/olap/rowset/segment_v2/segment_iterator.cpp

        RETURN_IF_ERROR(_column_iterators[_schema.unique_id(cid)]->get_row_ranges_by_zone_map(
                _opts.col_id_to_predicates[cid].get(),
-                _opts.col_id_to_del_predicates.count(cid) > 0
+                _opts.col_id_to_del_predicates.size() == 1 &&


If user send to delete predicates:

delete where a = 1

delete where a = 2

In this pr, the delete predicate will take no effect?

理论上，如果是两次独立的删除，比如delete from table where a=1; delete from table where a=2;其实这个地方应该可以使用的，但是目前的代码，是把所有不同版本的delete predicates和不同列的delete predicates都放到一起了，失去了版本信息、失去了谓词间可能是and的关系，统一弱化成了delete predicates都是独立的，有一个delete predicates满足条件，就把page都去掉。
这个pr的修改方式，就是在当前代码的基础上，当只有一个delete predicate的时候才能保证后续淘汰page的正确性，所以这里一律加了 == 1的判断才传递delete predicates。
如果要把不同版本的delete predicates和不同列的delete predicates作为完整和严谨的逻辑去判断page，需要修改的设计就有点多了，目前的方案算是一种优先解决bug的思路，后续可以进一步把delete predicates这块加速zone判断进行page淘汰的逻辑完善，提高delete predicates使用的场景。

yiguolei · 2023-02-27T02:31:28Z

regression-test/suites/delete_p0/test_zone_map_delete.groovy

+    "in_memory" = "false",
+    "storage_format" = "V2",
+    "light_schema_change" = "true",
+    "disable_auto_compaction" = "false"


I think should disable auto compaction, then p0 could produce stable result. If disable auto compaction == false, the rowset maybe merged.

ok，let me fix this test

yiguolei · 2023-02-27T02:53:16Z

be/src/olap/rowset/segment_v2/segment_iterator.cpp

        RETURN_IF_ERROR(_column_iterators[_schema.unique_id(cid)]->get_row_ranges_by_zone_map(
                _opts.col_id_to_predicates[cid].get(),
-                _opts.col_id_to_del_predicates.count(cid) > 0
+                _opts.col_id_to_del_predicates.size() == 1 &&


Add a comment here to explain these conditions. For example, why check size == 1

ok, i add a comment

nextdreamblue · 2023-02-27T06:50:06Z

run buildall

github-actions · 2023-02-27T06:52:14Z

clang-tidy review says "All clean, LGTM! 👍"

wangbo · 2023-02-27T07:27:54Z

be/src/olap/rowset/segment_v2/segment_iterator.cpp

+                // 统一弱化成了delete predicates都是独立的，有一个delete predicates满足条件，就把page都去掉。
+                // 这个pr的修改方式，就是在当前代码的基础上，当只有一个delete predicate的时候才能保证后续淘汰page的正确性，所以这里一律加了 == 1的判断才传递delete predicates。
+                // 如果要把不同版本的delete predicates和不同列的delete predicates作为完整和严谨的逻辑去判断page，需要修改的设计就有点多了，
+                // 目前的方案算是一种优先解决bug的思路，后续可以进一步把delete predicates这块加速zone判断进行page淘汰的逻辑完善，提高delete predicates使用的场景。


Please compact the comment and in English.
For example:

Currently, one transaction can only support one delete condition for page filtering. Following case is allowed: delete from table1 where a=1; delete from table1 where a=2; Following case is not allowed, because one transaction has more than on delete condition: delete from table1 where a=1 and b = 2; This may cause bug when you query table1 with a=1; You can refer #17145 for more details.

Please compact the comment and in English. For example:

Currently, one transaction can only support one delete condition for page filtering. Following case is allowed: delete from table1 where a=1; delete from table1 where a=2; Following case is not allowed, because one transaction has more than on delete condition: delete from table1 where a=1 and b = 2; This may cause bug when you query table1 with a=1; You can refer #17145 for more details.

thanks bro, you are my hero

nextdreamblue · 2023-02-27T07:33:36Z

run buildall

github-actions · 2023-02-27T07:35:43Z

clang-tidy review says "All clean, LGTM! 👍"

wangbo · 2023-02-27T08:17:12Z

be/src/olap/rowset/segment_v2/segment_iterator.cpp

        RETURN_IF_ERROR(_column_iterators[_schema.unique_id(cid)]->get_row_ranges_by_zone_map(
                _opts.col_id_to_predicates[cid].get(),
-                _opts.col_id_to_del_predicates.count(cid) > 0
+                // Currently, one transaction can only support one delete condition for page filtering.


It seems that when delete condition only contains OR predicates, page filter can work correctly here.
So this pr is a quick fix, it may cause some case performance degradation.
I think it's better to add a todo here.

ok, i add a todo

wangbo · 2023-02-27T08:23:09Z

regression-test/suites/delete_p0/test_zone_map_delete.groovy

+
+    sql """delete from ${tableName} where k2 is not null and k3=11;"""
+
+    qt_sql """select k2,k3 from ${tableName};"""


You'd better add order by in query sql.

it is better, lei fix it

wangbo

LGTM

nextdreamblue · 2023-02-27T08:40:50Z

run buildall

github-actions · 2023-02-27T08:40:59Z

PR approved by anyone and no changes requested.

github-actions · 2023-02-27T08:42:06Z

clang-tidy review says "All clean, LGTM! 👍"

nextdreamblue · 2023-02-27T09:13:52Z

run COMPILE

nextdreamblue · 2023-02-27T09:27:01Z

run buildall

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

github-actions · 2023-02-27T11:54:31Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

github-actions · 2023-02-27T12:01:40Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-02-27T12:06:40Z

clang-tidy review says "All clean, LGTM! 👍"

nextdreamblue · 2023-02-27T12:08:04Z

run buildall

yiguolei

LGTM

github-actions · 2023-02-28T01:19:48Z

PR approved by at least one committer and no changes requested.

理论上，如果是两次独立的删除，比如delete from table where a=1; delete from table where a=2;其实这个地方应该可以使用的，但是目前的代码，是把所有不同版本的delete predicates和不同列的delete predicates都放到一起了，失去了版本信息、失去了谓词间可能是and的关系，统一弱化成了delete predicates都是独立的，有一个delete predicates满足条件，就把page都去掉。这个pr的修改方式，就是在当前代码的基础上，当只有一个delete predicate的时候才能保证后续淘汰page的正确性，所以这里一律加了 == 1的判断才传递delete predicates。如果要把不同版本的delete predicates和不同列的delete predicates作为完整和严谨的逻辑去判断page，需要修改的设计就有点多了，目前的方案算是一种优先解决bug的思路，后续可以进一步把delete predicates这块加速zone判断进行page淘汰的逻辑完善，提高delete predicates使用的场景。

fix cherry-pick bug of #17146

…17146) 理论上，如果是两次独立的删除，比如delete from table where a=1; delete from table where a=2;其实这个地方应该可以使用的，但是目前的代码，是把所有不同版本的delete predicates和不同列的delete predicates都放到一起了，失去了版本信息、失去了谓词间可能是and的关系，统一弱化成了delete predicates都是独立的，有一个delete predicates满足条件，就把page都去掉。这个pr的修改方式，就是在当前代码的基础上，当只有一个delete predicate的时候才能保证后续淘汰page的正确性，所以这里一律加了 == 1的判断才传递delete predicates。如果要把不同版本的delete predicates和不同列的delete predicates作为完整和严谨的逻辑去判断page，需要修改的设计就有点多了，目前的方案算是一种优先解决bug的思路，后续可以进一步把delete predicates这块加速zone判断进行page淘汰的逻辑完善，提高delete predicates使用的场景。

* [fix](delete) fix delete from bug which can get wrong result (apache#17146) 理论上，如果是两次独立的删除，比如delete from table where a=1; delete from table where a=2;其实这个地方应该可以使用的，但是目前的代码，是把所有不同版本的delete predicates和不同列的delete predicates都放到一起了，失去了版本信息、失去了谓词间可能是and的关系，统一弱化成了delete predicates都是独立的，有一个delete predicates满足条件，就把page都去掉。这个pr的修改方式，就是在当前代码的基础上，当只有一个delete predicate的时候才能保证后续淘汰page的正确性，所以这里一律加了 == 1的判断才传递delete predicates。如果要把不同版本的delete predicates和不同列的delete predicates作为完整和严谨的逻辑去判断page，需要修改的设计就有点多了，目前的方案算是一种优先解决bug的思路，后续可以进一步把delete predicates这块加速zone判断进行page淘汰的逻辑完善，提高delete predicates使用的场景。 * [fix](delete) fix 'is null' or 'is not null' delete predicate will get wrong result (apache#17190) fix 'is null' or 'is not null' delete predicate will get wrong result Signed-off-by: nextdreamblue <zxw520blue1@163.com> --------- Signed-off-by: nextdreamblue <zxw520blue1@163.com> Co-authored-by: xueweizhang <zxw520blue1@163.com>

github-actions bot added the kind/test label Feb 26, 2023

yiguolei reviewed Feb 27, 2023

View reviewed changes

morningman added the dev/1.2.3 label Feb 27, 2023

yiguolei reviewed Feb 27, 2023

View reviewed changes

yiguolei added the usercase Important user case type label label Feb 27, 2023

wangbo reviewed Feb 27, 2023

View reviewed changes

wangbo previously approved these changes Feb 27, 2023

View reviewed changes

github-actions bot added the reviewed label Feb 27, 2023

nextdreamblue added 6 commits February 27, 2023 19:14

[fix](delete) fix delete from bug which can get wrong result

ab9da06

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

add regression-test

7dd5c25

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

add regression-test

926e2ed

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

fix code

7976bed

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

add comment

2f4779f

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

fix comment

8702623

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

nextdreamblue added 2 commits February 27, 2023 19:14

fix regression-test

6b52020

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

refactor code

97a9a0d

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

nextdreamblue dismissed wangbo’s stale review via 97a9a0d February 27, 2023 11:48

nextdreamblue force-pushed the fix_delete branch from 3a4297a to 97a9a0d Compare February 27, 2023 11:48

nextdreamblue added 2 commits February 27, 2023 19:55

fix code 3

b6bd92a

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

fix code 4

7b247e2

Signed-off-by: nextdreamblue <zxw520blue1@163.com>

yiguolei approved these changes Feb 28, 2023

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 28, 2023

yiguolei merged commit e0cd859 into apache:master Feb 28, 2023

morningman added dev/1.2.3-merged and removed dev/1.2.3 labels Feb 28, 2023

yiguolei added the dev/1.1.6 label Feb 28, 2023

nextdreamblue mentioned this pull request Feb 28, 2023

[fix](delete) fix delete from bug which can get wrong result #17226

Merged

5 tasks

morningman pushed a commit that referenced this pull request Feb 28, 2023

[fix](delete) fix delete from bug which can get wrong result (#17226)

f2a96fd

fix cherry-pick bug of #17146


		sql """delete from ${tableName} where k2 is not null and k3=11;"""

		qt_sql """select k2,k3 from ${tableName};"""

[fix](delete) fix delete from bug which can get wrong result #17146

[fix](delete) fix delete from bug which can get wrong result #17146

Uh oh!

Conversation

nextdreamblue commented Feb 26, 2023

Proposed changes

Problem summary

Checklist(Required)

Further comments

Uh oh!

github-actions bot commented Feb 26, 2023

Uh oh!

github-actions bot commented Feb 26, 2023

Uh oh!

github-actions bot commented Feb 26, 2023

Uh oh!

nextdreamblue commented Feb 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nextdreamblue Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangbo left a comment

Choose a reason for hiding this comment

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

nextdreamblue commented Feb 27, 2023

Uh oh!

yiguolei left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

nextdreamblue Feb 27, 2023 •

edited

Loading