Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(expr): support shuffle approx_percentile #17814

Merged
merged 18 commits into from
Jul 30, 2024
Merged

feat(expr): support shuffle approx_percentile #17814

merged 18 commits into from
Jul 30, 2024

Conversation

kwannoel
Copy link
Contributor

@kwannoel kwannoel commented Jul 25, 2024 •
edited
Loading

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. This is the unoptimized version which just uses a single datum to encode the state per group key.

We also just keep the entire state in-memory, rather than cache part of it.

For shuffle approx_percentile, we can just support it as part of the agg framework.

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

See #17873

@kwannoel kwannoel marked this pull request as draft July 25, 2024 08:43
@kwannoel kwannoel changed the title feat(expr): support single phase approx_percentile feat(expr): support shuffle approx_percentile Jul 25, 2024
@kwannoel kwannoel marked this pull request as ready for review July 26, 2024 02:09
@kwannoel kwannoel force-pushed the kwannoel/approx-percentile-single-phase branch from 2bcee12 to a74fa70 Compare July 26, 2024 05:19
@graphite-app graphite-app bot requested a review from xxchan July 26, 2024 08:26
wangrunji0408
wangrunji0408 approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@wangrunji0408 wangrunji0408 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM!

src/frontend/src/optimizer/plan_node/generic/agg.rs Outdated Show resolved Hide resolved
src/expr/impl/src/aggregate/approx_percentile.rs Outdated Show resolved Hide resolved
src/expr/impl/src/aggregate/approx_percentile.rs Outdated Show resolved Hide resolved
@kwannoel kwannoel requested a review from a team as a code owner July 29, 2024 07:05
@kwannoel
Copy link
Contributor Author

Re: Cargo-lock

I added bytes as a dependency to the risingwave_expr_impl crate.

stdrc
stdrc reviewed Jul 29, 2024
View reviewed changes
e2e_test/streaming/aggregate/approx_percentile.slt Outdated Show resolved Hide resolved
e2e_test/streaming/aggregate/approx_percentile.slt Outdated Show resolved Hide resolved
e2e_test/streaming/aggregate/approx_percentile.slt Show resolved Hide resolved
e2e_test/streaming/aggregate/approx_percentile.slt
-982.5779489474152 -804.4614206837127 0 804.4614206837127 982.5779489474152

# Test state encode / decode
onlyif can-use-recover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which cases we will set label can-use-recover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for e2e tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recover should not be used in cases where tests can execute in parallel.

@kwannoel
Copy link
Contributor Author

If LGTY could you leave a review on behalf of cargo-lock? @stdrc

@kwannoel kwannoel added this pull request to the merge queue Jul 30, 2024
Merged via the queue into main with commit f555be5 Jul 30, 2024
30 of 31 checks passed
@kwannoel kwannoel deleted the kwannoel/approx-percentile-single-phase branch July 30, 2024 03:43
@kwannoel kwannoel added the user-facing-changes Contains changes that are visible to users label Aug 2, 2024
@neverchanje neverchanje mentioned this pull request Aug 2, 2024
Closed
@WanYixian WanYixian mentioned this pull request Sep 4, 2024
Merged
2 tasks
@BugenZhao BugenZhao mentioned this pull request Sep 18, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stdrc stdrc stdrc approved these changes

@wangrunji0408 wangrunji0408 wangrunji0408 approved these changes

@fuyufjh fuyufjh Awaiting requested review from fuyufjh

@st1page st1page Awaiting requested review from st1page

@xxchan xxchan Awaiting requested review from xxchan

Assignees
No one assigned
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kwannoel @stdrc @wangrunji0408