-
Notifications
You must be signed in to change notification settings - Fork 599
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
fix(stream): add assertion to ensure pk indices consistency #12763
Conversation
94e66df
to
8efc56c
Compare
Codecov Report
@@ Coverage Diff @@
## main #12763 +/- ##
==========================================
+ Coverage 69.17% 69.25% +0.08%
==========================================
Files 1493 1493
Lines 246462 246466 +4
==========================================
+ Hits 170501 170701 +200
+ Misses 75961 75765 -196
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
You may need to rebase this branch. :) |
Signed-off-by: Richard Chien <[email protected]>
8efc56c
to
0169a00
Compare
Done. BTW, for others' information, although this is a fix, I guess we'd better not cherrypick this PR to v1.3-rc, as there may be cases not covered by our e2e tests. Let's wait for one release cycle to see if there is any. |
Signed-off-by: Richard Chien <[email protected]>
@@ -197,7 +196,7 @@ impl ExecutorBuilder for SourceExecutorBuilder { | |||
(0..source.columns.len()).collect_vec(), // eliminate the last column (_rw_offset) | |||
None, | |||
schema.clone(), | |||
pk_indices, |
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.
This is to fix https://buildkite.com/risingwavelabs/pull-request/builds/33652#018b407f-56a9-455f-bccf-9f0fc8eb6998. Plz help take a look @StrikeW.
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.
So for a source that has defined pk, does params.pk_indices
contains the index to the primary key or it is empty?
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.
Currently params.pk_indices
, which is derived by optimizer, is empty. So basically this change only ensures executor use the same pk_indices
as optimizer, the actual content of pk_indices
doesn't matter, because the Materialize
node at downstream has correct primary key. (I just want to keep changes minimal)
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.