Skip to content

Conversation

@BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Oct 17, 2025

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

What's changed and what's your intention?

With the introduction of the ChangeBuffer in the recent PR #23507, we can now streamline the implementation of executors that previously replicated this functionality, including:

  • MaterializeCache
  • TopNStaging
  • OverWindowExecutor

Actually, this was once proposed in #19451 (comment), where it seemed that we merged the PR in a rush and didn't get it done eventually.

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.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

@github-actions github-actions bot added the type/refactor Type: Refactoring. label Oct 17, 2025
Copy link
Member Author

BugenZhao commented Oct 17, 2025

@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch 2 times, most recently from 5f7166e to f4c1bbc Compare October 17, 2025 10:07
@BugenZhao BugenZhao force-pushed the bz/added-felidae branch 2 times, most recently from 00296c2 to a5b01a5 Compare October 21, 2025 06:17
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from f4c1bbc to c4aa20e Compare October 21, 2025 06:17
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from c4aa20e to 1ed8fec Compare October 21, 2025 06:41
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from 1ed8fec to 40945fd Compare October 21, 2025 10:14
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao changed the title refactor(streaming): use ChangeBuffer for more executors refactor(streaming): use ChangeBuffer for executors Oct 21, 2025
@BugenZhao BugenZhao marked this pull request as ready for review October 21, 2025 10:49
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from 8aca2f3 to 966e876 Compare October 21, 2025 11:08
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from 966e876 to 3bcf3d0 Compare October 22, 2025 05:58
Base automatically changed from bz/added-felidae to main October 22, 2025 13:49
@buildkite-limited-access buildkite-limited-access bot requested a review from a team as a code owner October 22, 2025 13:49
@buildkite-limited-access buildkite-limited-access bot requested review from MrCroxx and removed request for a team October 22, 2025 13:49
@BugenZhao BugenZhao force-pushed the bz/change-buffer-top-n branch from 3bcf3d0 to f2c4203 Compare October 23, 2025 06:12
Comment on lines -889 to -892
// We expect one `CacheKey` to appear at most once in the staging, and, the order of
// the outputs of `TopN` doesn't really matter, so we can simply chain the three maps.
// Although the output order is not important, we still ensure that `Delete`s are emitted
// before `Insert`s, so that we can avoid temporary violation of the `LIMIT` constraint.
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't matter any more as we switch internal to IndexMap.

The caller guarantees that when updating a row in staging, it always call delete before insert. Thus, by iterating the changes based on the operation order, we won't violate the LIMIT either.

@BugenZhao BugenZhao requested review from hzxa21 and removed request for MrCroxx October 23, 2025 06:25
@BugenZhao BugenZhao requested review from chenzl25 and Copilot October 23, 2025 06:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors streaming executors to use the centralized ChangeBuffer utility instead of maintaining duplicated change-buffering logic. The change eliminates redundant implementations in MaterializeCache, TopNStaging, and OverWindowExecutor, improving code maintainability and consistency.

Key Changes:

  • Replaced custom change buffering logic with ChangeBuffer in multiple executors
  • Added try_map and map methods to Record<R> for row transformation
  • Updated executors to use Record types and append_record instead of tuples

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/stream/src/common/change_buffer.rs Added Default trait implementation for ChangeBuffer
src/common/src/array/stream_record.rs Added try_map and map methods to Record<R> for row transformations
src/stream/src/executor/mview/materialize.rs Removed local ChangeBuffer implementation in favor of common ChangeBuffer; removed generate_output function
src/stream/src/executor/over_window/general.rs Replaced manual change merging logic with ChangeBuffer
src/stream/src/executor/top_n/top_n_cache.rs Refactored TopNStaging to use ChangeBuffer internally; simplified insert/delete operations
src/stream/src/executor/top_n/top_n_plain.rs Updated to use Record type and append_record method
src/stream/src/executor/top_n/top_n_appendonly.rs Updated to use Record type and append_record method
src/stream/src/executor/top_n/group_top_n_appendonly.rs Added explicit type annotation for stagings map; updated to use append_record
src/stream/src/executor/top_n/group_top_n.rs Added explicit type annotation for stagings map; updated to use append_record

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

@BugenZhao BugenZhao added this pull request to the merge queue Oct 23, 2025
Merged via the queue into main with commit 0610624 Oct 23, 2025
34 checks passed
@BugenZhao BugenZhao deleted the bz/change-buffer-top-n branch October 23, 2025 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/refactor Type: Refactoring.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants