feat(frontend): support two phase vnode based simple agg with approx_percentile #18007

kwannoel · 2024-08-12T14:33:52Z

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

What's changed and what's your intention?

As per title. Previously only stateless agg is supported. Now we support two phase vnode based agg too.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

kwannoel · 2024-08-12T14:34:20Z

e2e_test/streaming/aggregate/two_phase_approx_percentile_merge_stateless_agg.slt

-select * from m1;
----
-982.5779489474152 0 0 2001 982.5779489474152
-


Don't need this extra recovery step. We already did it in the lines before. Hence we can just remove it.

kwannoel · 2024-08-12T14:34:32Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

-        let approx_percentile =
-            self.build_approx_percentile_aggs(core.input.clone(), &approx_percentile_agg_calls)?;
-
-        // ====== Handle normal aggs


Refactored into a separate function.

kwannoel · 2024-08-12T14:34:42Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

-            }
-        } else {
-            Ok(global_agg.into())
-        }


Refactored into a separate function.

kwannoel · 2024-08-12T14:35:20Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+                return Ok(approx_percentile);
+            };
+            bail!("expected at least one agg call");
+        }


Can just reuse the logic via the refactored functions above, from two phase stateless agg.

kwannoel · 2024-08-12T14:35:27Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+            global_agg,
+            approx_percentile_col_mapping,
+            non_approx_percentile_col_mapping,
+        )


Can just reuse the logic via the refactored functions above, from two phase stateless agg.

kwannoel · 2024-08-12T14:36:06Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+    /// Prepares metadata and the `approx_percentile` plan, if there's one present.
+    fn prepare_approx_percentile(
+        &self,
+        core: &mut Agg<PlanRef>,
+        stream_input: PlanRef,
+    ) -> Result<(bool, ColIndexMapping, ColIndexMapping, Option<PlanRef>)> {
+        let SeparatedAggInfo { normal, approx } = self.separate_normal_and_special_agg();
+
+        let AggInfo {
+            calls: non_approx_percentile_agg_calls,
+            col_mapping: non_approx_percentile_col_mapping,
+        } = normal;
+        let AggInfo {
+            calls: approx_percentile_agg_calls,
+            col_mapping: approx_percentile_col_mapping,
+        } = approx;
+        if !self.group_key().is_empty() && !non_approx_percentile_agg_calls.is_empty() {
+            bail_not_implemented!("two-phase approx percentile agg with group key, please use single phase agg for approx_percentile with group key");
+        }
+
+        let needs_row_merge = (!non_approx_percentile_agg_calls.is_empty()
+            && !approx_percentile_agg_calls.is_empty())
+            || approx_percentile_agg_calls.len() >= 2;
+        core.input = if needs_row_merge {
+            // If there's row merge, we need to share the input.
+            StreamShare::new_from_input(stream_input.clone()).into()
+        } else {
+            stream_input
+        };
+        core.agg_calls = non_approx_percentile_agg_calls;
+
+        let approx_percentile =
+            self.build_approx_percentile_aggs(core.input.clone(), &approx_percentile_agg_calls)?;
+        Ok((
+            needs_row_merge,
+            non_approx_percentile_col_mapping,
+            approx_percentile_col_mapping,
+            approx_percentile,
+        ))
+    }
+
+    /// Add `RowMerge` if needed
+    fn add_row_merge_if_needed(
+        needs_row_merge: bool,
+        approx_percentile: Option<PlanRef>,
+        global_agg: PlanRef,
+        approx_percentile_col_mapping: ColIndexMapping,
+        non_approx_percentile_col_mapping: ColIndexMapping,
+    ) -> Result<PlanRef> {
+        if let Some(approx_percentile) = approx_percentile {
+            if needs_row_merge {
+                let row_merge = StreamRowMerge::new(
+                    approx_percentile,
+                    global_agg,
+                    approx_percentile_col_mapping,
+                    non_approx_percentile_col_mapping,
+                )?;
+                Ok(row_merge.into())
+            } else {
+                Ok(approx_percentile)
+            }
+        } else {
+            Ok(global_agg)
+        }
+    }


The refactored functionalities from above.

kwannoel · 2024-08-12T14:36:36Z

e2e_test/streaming/aggregate/two_phase_approx_percentile_merge_stateful_agg.slt

+create materialized view m1 as
+ select
+     approx_percentile(0.01, 0.01) within group (order by p_col) as p01,
+     min(p_col),


Here we test a stateful agg, so we can test the two phase vnode based simple agg path.

Can we add a planner test for this case as well?

chenzl25

Rest LGTM

chenzl25 · 2024-08-14T03:53:16Z

e2e_test/streaming/aggregate/two_phase_approx_percentile_merge_stateful_agg.slt

+create materialized view m1 as
+ select
+     approx_percentile(0.01, 0.01) within group (order by p_col) as p01,
+     min(p_col),


Can we add a planner test for this case as well?

chenzl25 · 2024-08-14T04:05:30Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+                )?;
+                Ok(row_merge.into())
+            } else {
+                Ok(approx_percentile)


Is it a reachable path? Inputs indicate that we have a global_agg, but we discard it here.

chenzl25 · 2024-08-14T06:20:46Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+                bail!("row_merge not needed, but approx_percentile and normal agg are present");
+            }


If that is the case, does it mean we don't need this flag needs_row_merge? Because we can determine whether to merge by approx_percentile: Option<PlanRef>.

Yeah that's true. Simplified it 21019ae

chenzl25

Rest LGTM

chenzl25 · 2024-08-14T08:58:38Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

@@ -304,6 +291,68 @@ impl LogicalAgg {
        }
    }

+    /// Prepares metadata and the `approx_percentile` plan, if there's one present.
+    fn prepare_approx_percentile(


This function would modify the core.agg_calls and core.input. We'd better document this behavior.

kwannoel added 2 commits August 12, 2024 21:49

add test

78de14e

implement two phase stateful approx_percentile agg

dd330a5

github-actions bot added the type/feature label Aug 12, 2024

kwannoel commented Aug 12, 2024

View reviewed changes

kwannoel changed the title ~~feat(frontend): support two phase vnode based agg with approx_percentile~~ feat(frontend): support two phase vnode based simple agg with approx_percentile Aug 12, 2024

kwannoel requested review from stdrc, chenzl25 and st1page August 12, 2024 14:36

fix

995c427

graphite-app bot requested a review from a team August 13, 2024 02:11

chenzl25 reviewed Aug 14, 2024

View reviewed changes

add planner test + bail on invalid branch

ce410f4

chenzl25 reviewed Aug 14, 2024

View reviewed changes

simplify

21019ae

chenzl25 approved these changes Aug 14, 2024

View reviewed changes

comments

50576a5

kwannoel enabled auto-merge August 14, 2024 09:10

kwannoel added this pull request to the merge queue Aug 14, 2024

Merged via the queue into main with commit f09f195 Aug 14, 2024
30 of 31 checks passed

kwannoel deleted the kwannoel/approx-percentile-stateful branch August 14, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): support two phase vnode based simple agg with approx_percentile #18007

feat(frontend): support two phase vnode based simple agg with approx_percentile #18007

kwannoel commented Aug 12, 2024

kwannoel Aug 12, 2024 •

edited

Loading

kwannoel Aug 12, 2024

kwannoel Aug 12, 2024

kwannoel Aug 12, 2024

kwannoel Aug 12, 2024

kwannoel Aug 12, 2024

kwannoel Aug 12, 2024

chenzl25 Aug 14, 2024

kwannoel Aug 14, 2024

chenzl25 left a comment

chenzl25 Aug 14, 2024

chenzl25 Aug 14, 2024 •

edited

Loading

kwannoel Aug 14, 2024

chenzl25 Aug 14, 2024

kwannoel Aug 14, 2024

chenzl25 left a comment

chenzl25 Aug 14, 2024

		bail!("row_merge not needed, but approx_percentile and normal agg are present");
		}

feat(frontend): support two phase vnode based simple agg with approx_percentile #18007

feat(frontend): support two phase vnode based simple agg with approx_percentile #18007

Conversation

kwannoel commented Aug 12, 2024

What's changed and what's your intention?

Checklist

Documentation

Release note

kwannoel Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

chenzl25 Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

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

kwannoel Aug 12, 2024 •

edited

Loading

chenzl25 Aug 14, 2024 •

edited

Loading