feat(frontend): Supports cut OR condition and push down to storage #19812

Li0k · 2024-12-16T12:05:21Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

related to #19525

This PR implements the optimizations mentioned in the issue

try to merge scan_range with same eq.
try to push down multiple scan ranges instead of full table scan

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

…nto li0k/batch_predicate_pushdown

st1page · 2024-12-18T04:11:21Z

src/frontend/planner_test/tests/testdata/output/range_scan.yaml

@@ -418,7 +418,7 @@
  batch_plan: |-
    BatchExchange { order: [], dist: Single }
    └─BatchFilter { predicate: (((orders_count_by_user.user_id = 1:Int32) OR ((orders_count_by_user.user_id = 2:Int32) AND In(orders_count_by_user.date, 1111:Int32, 2222:Int32))) OR (orders_count_by_user.user_id <> 3:Int32)) }
-      └─BatchScan { table: orders_count_by_user, columns: [orders_count_by_user.user_id, orders_count_by_user.date, orders_count_by_user.orders_count], distribution: UpstreamHashShard(orders_count_by_user.user_id, orders_count_by_user.date) }
+      └─BatchScan { table: orders_count_by_user, columns: [orders_count_by_user.user_id, orders_count_by_user.date, orders_count_by_user.orders_count], scan_ranges: [orders_count_by_user.user_id = Int64(1), orders_count_by_user.user_id = Int64(2) AND orders_count_by_user.date = Int32(1111), orders_count_by_user.user_id = Int64(2) AND orders_count_by_user.date = Int32(2222)], distribution: UpstreamHashShard(orders_count_by_user.user_id, orders_count_by_user.date) }


I think this can not be pushed down with condition orders_count_by_user.user_id <> 3:Int32?

Good catch, I didn't realize that full table scan range would be converted to empty vec, and I fixed it with a special judgment.

…nto li0k/batch_predicate_pushdown

Li0k · 2024-12-18T11:05:55Z

Test:

CREATE TABLE t1(v1 int primary key);

INSERT INTO t1 (v1) SELECT v FROM generate_series(1, 10000000) AS v;

SELECT COUTN(*) FROM t1;

SELECT * FROM t1 WHERE (v1 > 10 AND v1 < 20) OR (v1 > 5000000 AND v1 < 5000100);

before:

after:

ZENOTME · 2024-12-23T05:22:45Z

src/frontend/src/utils/condition.rs

+
+                scan_ranges.extend(scan_ranges_chunk);
+            }
+            scan_ranges.sort_by(|a, b| a.eq_conds.len().cmp(&b.eq_conds.len()));


Should we also sort lower_bound for each range here?🤔

e.g. SELECT * FROM orders_count_by_user WHERE (user_id < 10) or (user_id > 30) or (user_id > 5 and user_id < 15);
got following plan:

batch_plan: |- BatchExchange { order: [], dist: Single } └─BatchFilter { predicate: (((orders_count_by_user.user_id < 10:Int32) OR (orders_count_by_user.user_id > 30:Int32)) OR ((orders_count_by_user.user_id > 5:Int32) AND (orders_count_by_user.user_id < 15:Int32))) } └─BatchScan { table: orders_count_by_user, columns: [orders_count_by_user.user_id, orders_count_by_user.date, orders_count_by_user.orders_count], scan_ranges: [orders_count_by_user.user_id < Int64(10), orders_count_by_user.user_id > Int64(30), orders_count_by_user.user_id > Int64(5) AND orders_count_by_user.user_id < Int64(15)], distribution: UpstreamHashShard(orders_count_by_user.user_id, orders_count_by_user.date) }

I think what we expect is:

batch_plan: |- BatchExchange { order: [], dist: Single } └─BatchFilter { predicate: (((orders_count_by_user.user_id < 10:Int32) OR (orders_count_by_user.user_id > 30:Int32)) OR ((orders_count_by_user.user_id > 5:Int32) AND (orders_count_by_user.user_id < 15:Int32))) } └─BatchScan { table: orders_count_by_user, columns: [orders_count_by_user.user_id, orders_count_by_user.date, orders_count_by_user.orders_count], scan_ranges: [orders_count_by_user.user_id < Int64(15), orders_count_by_user.user_id > Int64(30)], distribution: UpstreamHashShard(orders_count_by_user.user_id, orders_count_by_user.date) }

I try to reorder the predicate: SELECT * FROM orders_count_by_user WHERE (user_id < 10) or (user_id > 5 and user_id < 15) or (user_id > 30);

batch_plan: |- BatchExchange { order: [], dist: Single } └─BatchFilter { predicate: (((orders_count_by_user.user_id < 10:Int32) OR ((orders_count_by_user.user_id > 5:Int32) AND (orders_count_by_user.user_id < 15:Int32))) OR (orders_count_by_user.user_id > 30:Int32)) } └─BatchScan { table: orders_count_by_user, columns: [orders_count_by_user.user_id, orders_count_by_user.date, orders_count_by_user.orders_count], scan_ranges: [orders_count_by_user.user_id <= Int64(15), orders_count_by_user.user_id > Int64(30)], distribution: UpstreamHashShard(orders_count_by_user.user_id, orders_count_by_user.date) }

Why we get orders_count_by_user.user_id <= Int64(15) rather than orders_count_by_user.user_id < Int64(15) here?🤔

Indeed we need to sort the lower bound instead of equal condition here.

chenzl25

Rest LGTM

chenzl25 · 2024-12-23T06:52:13Z

src/frontend/src/utils/condition.rs

+
+                scan_ranges.extend(scan_ranges_chunk);
+            }
+            scan_ranges.sort_by(|a, b| a.eq_conds.len().cmp(&b.eq_conds.len()));


Indeed we need to sort the lower bound instead of equal condition here.

…nto li0k/batch_predicate_pushdown

Li0k · 2024-12-23T08:49:28Z

fix it , PTAL @chenzl25 @ZENOTME

graphite-app · 2024-12-23T08:50:13Z

src/common/src/util/scan_range.rs

@@ -98,6 +100,138 @@ impl ScanRange {
            range: full_range(),
        }
    }
+
+    pub fn covert_to_range(&self) -> (Bound<Vec<Datum>>, Bound<Vec<Datum>>) {


The function name covert_to_range appears to be a typo and should be convert_to_range. This typo is propagated in the function's usage on lines 133 and 371. Consider fixing all instances to maintain consistency and readability.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-23T08:52:09Z

src/common/src/util/scan_range.rs

+        if left_start_vec.is_empty() && right_start_vec.is_empty() {
+            return true;
+        }


When both start vectors are empty, the code returns true without checking the end bounds. This could incorrectly mark ranges as overlapping when they don't actually overlap. Consider this case:

Range 1: (-∞, 5] Range 2: (-∞, 3]

These ranges overlap, but:

Range 1: (-∞, 3] Range 2: (5, ∞)

These ranges don't overlap despite both having empty start vectors. The end bounds need to be checked in all cases.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

chenzl25 · 2024-12-23T10:36:39Z

src/frontend/planner_test/tests/testdata/input/range_scan.yaml

@@ -428,3 +428,54 @@
  expected_outputs:
  - logical_plan
  - batch_plan
+- name: When OR clauses contain non-overlapping conditions,, we can pushdown serveral scan_range.
+  before:
+  - create_table_and_mv


Suggests adding some tests in descending order.

Good catch, We don't need to consider the direction of the ordering in the range comparison in this pr, so we change all the defaults to asce to simplify the comparison logic.

…nto li0k/batch_predicate_pushdown

chenzl25

LGTM! Thanks for your PR!

…nto li0k/batch_predicate_pushdown

…19812)

Li0k added 2 commits December 16, 2024 19:55

feat(batch): Supports cut or condition and push down to storage

571e85e

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

30d2aaf

…nto li0k/batch_predicate_pushdown

Li0k requested review from chenzl25 and xxhZs December 16, 2024 12:05

github-actions bot added the type/feature label Dec 16, 2024

Li0k changed the title ~~feat(batch): Supports cut OR condition and push down to storage~~ WIP: feat(batch): Supports cut OR condition and push down to storage Dec 16, 2024

github-actions bot added the Invalid PR Title label Dec 16, 2024

Li0k marked this pull request as ready for review December 16, 2024 12:08

graphite-app bot requested a review from a team December 16, 2024 12:28

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

83d248a

…nto li0k/batch_predicate_pushdown

Li0k changed the title ~~WIP: feat(batch): Supports cut OR condition and push down to storage~~ WIP: feat(frontend): Supports cut OR condition and push down to storage Dec 17, 2024

Li0k changed the title ~~WIP: feat(frontend): Supports cut OR condition and push down to storage~~ feat(frontend): Supports cut OR condition and push down to storage Dec 18, 2024

github-actions bot removed the Invalid PR Title label Dec 18, 2024

Li0k requested a review from st1page December 18, 2024 03:40

st1page reviewed Dec 18, 2024

View reviewed changes

Li0k added 3 commits December 18, 2024 15:35

fix(frontend): fix buggy pushdown

ae6736b

fix(frontend): overlap bug fix and add more test

fe5c2b9

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

203e198

…nto li0k/batch_predicate_pushdown

Li0k requested a review from ZENOTME December 23, 2024 04:00

ZENOTME reviewed Dec 23, 2024

View reviewed changes

chenzl25 reviewed Dec 23, 2024

View reviewed changes

Li0k added 3 commits December 23, 2024 16:40

fix(frontend): fix ordering of scan range

a851fbc

fix check

7c62b53

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

e2ff69d

…nto li0k/batch_predicate_pushdown

graphite-app bot reviewed Dec 23, 2024

View reviewed changes

fix check

41105a4

graphite-app bot reviewed Dec 23, 2024

View reviewed changes

chenzl25 reviewed Dec 23, 2024

View reviewed changes

Li0k added 3 commits December 24, 2024 15:25

fix(frontend): fix desc ordering

ce9fbc2

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

fb87fbd

…nto li0k/batch_predicate_pushdown

typo

f8d0a24

chenzl25 approved these changes Dec 24, 2024

View reviewed changes

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

0fbb807

…nto li0k/batch_predicate_pushdown

Li0k enabled auto-merge December 24, 2024 11:59

Li0k added this pull request to the merge queue Dec 24, 2024

Merged via the queue into main with commit 3431eab Dec 24, 2024
28 of 29 checks passed

Li0k deleted the li0k/batch_predicate_pushdown branch December 24, 2024 13:04

wcy-fdu added the need-cherry-pick-release-2.1 label Dec 24, 2024

github-actions bot mentioned this pull request Dec 24, 2024

cherry-pick feat(frontend): Supports cut OR condition and push down to storage (#19812) to branch release-2.1 #19923

Closed

Li0k added a commit that referenced this pull request Dec 25, 2024

feat(frontend): Supports cut OR condition and push down to storage (#…

edd8fa6

…19812)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): Supports cut OR condition and push down to storage #19812

feat(frontend): Supports cut OR condition and push down to storage #19812

Li0k commented Dec 16, 2024

st1page Dec 18, 2024

Li0k Dec 18, 2024

Li0k commented Dec 18, 2024

ZENOTME Dec 23, 2024

ZENOTME Dec 23, 2024

chenzl25 Dec 23, 2024

chenzl25 left a comment

chenzl25 Dec 23, 2024

Li0k commented Dec 23, 2024

graphite-app bot Dec 23, 2024

graphite-app bot Dec 23, 2024

chenzl25 Dec 23, 2024

Li0k Dec 24, 2024

chenzl25 left a comment

feat(frontend): Supports cut OR condition and push down to storage #19812

feat(frontend): Supports cut OR condition and push down to storage #19812

Conversation

Li0k commented Dec 16, 2024

What's changed and what's your intention?

Checklist

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Dec 23, 2024

graphite-app bot Dec 23, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment