Skip to content

Conversation

xxchan
Copy link
Collaborator

@xxchan xxchan commented Jul 16, 2025

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

What's changed and what's your intention?

First part of #22690. See the tracking issue for the whole picture of the user journey.

Frontend part.

Key changes include:

  • Added a refreshable flag to the table catalog to indicate whether a table supports refresh operations. In this PR, there's no way to set the flag yet.
  • Implemented a new REFRESH TABLE SQL command and corresponding handler. It takes effect for table with refreshable flag (so no way to use it in this PR).
  • Added new barrier mutations RefreshStart (send by REFRESH TABLE SQL) and LoadFinish (will have a new mechanism to send it, introduced in the following PR.)

Checklist

  • I have written necessary rustdoc comments.

Copy link
Collaborator Author

xxchan commented Jul 16, 2025

@xxchan xxchan force-pushed the xxchan/protective-jackal branch from 75e6f3a to 09dedc4 Compare July 22, 2025 05:37
@xxchan xxchan force-pushed the xxchan/protective-jackal branch from 09dedc4 to 5b518e5 Compare July 22, 2025 06:42
@xxchan xxchan changed the base branch from main to graphite-base/22619 July 22, 2025 08:35
@xxchan xxchan force-pushed the xxchan/protective-jackal branch from 5b518e5 to a409585 Compare July 22, 2025 08:35
@xxchan xxchan changed the base branch from graphite-base/22619 to 07-22-feat_minor_improve_risedev_d_in_cursorrule July 22, 2025 08:35
Comment on lines +205 to +207
fn is_refreshable_connector(&self) -> bool {
false
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This decides whether a table can be refreshed. So not possible to create refreshable table in this PR

Base automatically changed from 07-22-feat_minor_improve_risedev_d_in_cursorrule to main July 22, 2025 09:19
@xxchan xxchan force-pushed the xxchan/protective-jackal branch from a409585 to 6f04b2b Compare July 22, 2025 10:07
@xxchan xxchan marked this pull request as ready for review July 23, 2025 02:10
Copy link

graphite-app bot commented Jul 23, 2025

Looks like this PR extends new SQL syntax or updates existing ones. Make sure that:

  • Test cases about the new/updated syntax are added in src/sqlparser/tests/testdata. Especially, double check the formatted_sql is still a valid SQL #20713
  • The meaning of each enum variant is documented in PR description. Additionally, document what it means when each optional clause is omitted.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Refresh Syntax Parsing Error

The REFRESH statement parser incorrectly mandates the TABLE keyword (REFRESH TABLE <table_name>). This conflicts with existing test data and the AST display format, which both imply REFRESH <table_name> is the expected syntax, leading to parsing failures for the simpler form.

src/sqlparser/src/parser.rs#L377-L381

pub fn parse_refresh(&mut self) -> ModalResult<Statement> {
self.expect_keyword(Keyword::TABLE)?;
let table_name = self.parse_object_name()?;
Ok(Statement::Refresh { table_name })
}

Fix in CursorFix in Web


Bugbot free trial expires on July 29, 2025
Learn more in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

@xxchan xxchan force-pushed the xxchan/protective-jackal branch 4 times, most recently from 553fc73 to 23ca770 Compare July 23, 2025 05:19
@xxchan xxchan force-pushed the xxchan/protective-jackal branch from 23ca770 to 03a7909 Compare July 24, 2025 05:24
@xxchan xxchan requested review from BugenZhao and chenzl25 July 25, 2025 06:21
Comment on lines +132 to +135
message LoadFinishMutation {
// Associated source ID for this load operation.
uint32 associated_source_id = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need table_id here as RefreshStartMutation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I think for RefreshStartMutation we also just need the source id, because currently StreamSource only has source_id, but not the corresponding table id, while MaterializeNode has the catalog.Table, containing both table_id and associated_source_id. 🤣

/// 1. Validates that the table exists and is refreshable
/// 2. Sends a refresh command through the barrier system
/// 3. Returns the result of the refresh operation
pub async fn refresh_table(&self, request: RefreshRequest) -> MetaResult<RefreshResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

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

When manually triggering a refresh command, do we need to send the loadFinish command as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The loadFinish command is triggered after SourceExec reporting data load finished. added in #22620

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

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

/// 1. Validates that the table exists and is refreshable
/// 2. Sends a refresh command through the barrier system
/// 3. Returns the result of the refresh operation
pub async fn refresh_table(&self, request: RefreshRequest) -> MetaResult<RefreshResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

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.

Rest LGTM

Comment on lines +75 to +80
// Refresh command sent successfully
tracing::info!(
table_id = %table_id,
table_name = %table_name,
"Manual refresh initiated"
);
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that we don't wait for REFRESH to be finished?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. But in the future, we can make it a blocking command

@xxchan xxchan enabled auto-merge July 28, 2025 07:12
@xxchan xxchan added this pull request to the merge queue Jul 28, 2025
Merged via the queue into main with commit 1d0947a Jul 28, 2025
35 of 36 checks passed
@xxchan xxchan deleted the xxchan/protective-jackal branch July 28, 2025 08:27
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.

3 participants