-
Notifications
You must be signed in to change notification settings - Fork 590
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(steam): support stream change log #17132
Conversation
33d9a2e
to
5195a4e
Compare
ba7bc10
to
d940439
Compare
d940439
to
10a7707
Compare
input: PlanRef, | ||
input_col_change: ColIndexMapping, | ||
) -> (Self, ColIndexMapping) { | ||
let changed_log = Self::new(input, self.core.need_op, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this logic to logical_rewrite_for_stream
, because it has done more things than rewrite_with_input
should contain.
@@ -287,6 +287,10 @@ message FilterNode { | |||
expr.ExprNode search_condition = 1; | |||
} | |||
|
|||
message ChangedLogNode { | |||
bool need_op = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is used for projection push-down to prune out op column if it is not used in downstream. Can we have some documentation to explain in which case need_op
is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add documentation here in the pb in addition to the rust codes as well?
@@ -133,7 +133,7 @@ impl<'a, R: Rng> SqlGenerator<'a, R> { | |||
let from = None; | |||
let cte = Cte { | |||
alias: alias.clone(), | |||
query, | |||
query: Some(query), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How hard it is to support changelog cte in sqlsmith as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do it in next pr
I did a simple test with this PR: dev=> create table t (v1 int primary key, v2 int);
CREATE_TABLE
dev=> create materialized view mv as select * from t;
CREATE_MATERIALIZED_VIEW
dev=> create materialized view changelog as with c as changedlog from mv select * from c;
CREATE_MATERIALIZED_VIEW
dev=> describe changelog;
Name | Type | Is Hidden | Description
---------------------+-------------------------+-----------+-------------
v1 | integer | false |
v2 | integer | false |
op | smallint | false |
_changed_log_row_id | serial | true |
primary key | v1, _changed_log_row_id | |
distribution key | _changed_log_row_id | |
table description | changelog | |
(7 rows) Is it expected that the pk of |
fixed ,now pk is only _changed_log_row_id |
let keys = vec![self.schema().len() - 1]; | ||
Some(keys) | ||
} else { | ||
None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, change_log
's stream key must be not None and must be its row_id. In which case will we reach this branch?
Tried with another example:
Is it expected to include |
Another failing example:
chaining changelog on top of another changelog MV |
0e6ee46
to
ab6422a
Compare
}; | ||
yield Message::Chunk(new_chunk); | ||
} | ||
m => yield m, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For watermark message, should we also convert it to a new kind of row, or just discard the watermark? The semantic of watermark will be broken if we just simply yield the original watermark.
bbba5e0
to
0b21756
Compare
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
9425213 | Triggered | Generic Password | 0b21756 | ci/scripts/e2e-sink-test.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM. Please update the Release Note as well.
@@ -287,6 +287,10 @@ message FilterNode { | |||
expr.ExprNode search_condition = 1; | |||
} | |||
|
|||
message ChangedLogNode { | |||
bool need_op = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add documentation here in the pb in addition to the rust codes as well?
Several minor fixes:
I will leave it to you to decide whether they are worth cherry-picking. |
+1 for cherry-picking because they are all valid bug fixes. |
+1 for cherry-picking and some user meet it. https://risingwave-community.slack.com/archives/C05KL806L3Z/p1729573860975309 |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
add an changed log exec
use cte to create changed log
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
in this pr, we support the stream changelog. It can send the 'op' on the stream downstream as a separate column
After this operation, we will discard the upstream pk and recreate a self-incrementing id _change_log_row_id. And the table upstream of changelog cannot have the same name as changelog_op.
Syntactically, we use a cte-like syntax
example
the result is
v1 v2 changelog_op
1 1 1
2 2 1
2 2 4
2 100 3
2 2 2