Skip to content

Conversation

@BugenZhao
Copy link
Member

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

What's changed and what's your intention?

There is no clear definition of watermarks for upsert/retract streams, and our current impl doesn't expect this to happen as well.

Previously, a SOURCE had to be append-only, allowing us to apply the WatermarkFilter directly once a user defined a watermark, without any checks. However, since we introduced support for upsert sources in #22856, this assumption is no longer valid, and we need to implement this check.

This PR also add tests for the error handling of defining watermarks on non-append-only tables.

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/fix Type: Bug fix. Only for pull requests. label Dec 15, 2025
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +39 to +43
assert!(
input.append_only(),
"StreamWatermarkFilter only supports append-only input, got {}",
input.stream_kind()
);

Choose a reason for hiding this comment

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

P1 Badge Handle non-append-only inputs in StreamWatermarkFilter

The new assert!(input.append_only()) in StreamWatermarkFilter::new will panic whenever a source carries watermarks but its stream kind is upsert/retract. Older clusters could already contain such sources (the preceding binder check only blocks new ones), and planning a new MV on top of them or re-planning during recovery would now crash the frontend instead of returning a user-facing error. Consider returning a structured ErrorCode here or rejecting the plan earlier so existing non-append-only sources with watermarks don’t bring down the service.

Useful? React with 👍 / 👎.

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

Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao
Copy link
Member Author

This change is part of the following stack:

Change managed by git-spice.

@BugenZhao BugenZhao added this pull request to the merge queue Dec 16, 2025
Merged via the queue into main with commit 878373f Dec 16, 2025
34 checks passed
@BugenZhao BugenZhao deleted the bz/strict-magpie branch December 16, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/fix Type: Bug fix. Only for pull requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants