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.

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

feat: use order key as mv's dist key #20176

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Conversation

st1page
Copy link
Contributor

@st1page st1page commented Jan 15, 2025

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

What's changed and what's your intention?

close #19321

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

Comment on lines +28 to +31
└─StreamExchange { dist: HashShard(t1.v1) }
└─StreamTopN [append_only] { order: [t1.v1 ASC], limit: 3, offset: 3 }
└─StreamExchange { dist: Single }
└─StreamTableScan { table: t1, columns: [t1.v1, t1._row_id], stream_scan_type: ArrangementBackfill, stream_key: [t1._row_id], pk: [_row_id], dist: UpstreamHashShard(t1._row_id) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know if we should make some special behavior for the order by clause with limit... Maybe it is ok because there are not many people write a ToN streaming job and has expectation on its performance

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@st1page st1page requested review from chenzl25 and BugenZhao January 15, 2025 09:15
@st1page st1page marked this pull request as draft January 15, 2025 13:14
@st1page st1page marked this pull request as ready for review January 16, 2025 08:33
Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks~

…gwavelabs/risingwave into sts/distribution_key_with_order_by
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Could you show that in which cases we will gain performance benefits from this change?

@BugenZhao BugenZhao requested review from stdrc and xxchan January 17, 2025 07:10
@st1page st1page enabled auto-merge January 17, 2025 08:05
@st1page
Copy link
Contributor Author

st1page commented Jan 17, 2025

Could you show that in which cases we will gain performance benefits from this change?

I think the main discussion is in the issue and hard to give a overall summary.

@st1page st1page added this pull request to the merge queue Jan 17, 2025
@BugenZhao
Copy link
Member

Could you show that in which cases we will gain performance benefits from this change?

I think the main discussion is in the issue and hard to give a overall summary.

Is there at least any simple example? 🤣

Merged via the queue into main with commit 27efddf Jan 17, 2025
28 of 29 checks passed
@st1page st1page deleted the sts/distribution_key_with_order_by branch January 17, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Use ORDER BY as part of MV's distribution key
3 participants