-
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(frontend): properly handled insert
given misaligned source
#9208
Conversation
insert
given misaligned source
.0 | ||
.first() | ||
.expect("values list should not be empty") | ||
.len(); | ||
let values = self.bind_values(values.clone(), Some(expected_types))?; |
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.
since we decided to punt the NULL padding to the batch executor, perhaps the NULL padding logic in bing_values
should be deleted
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.
True, actually I just forgot to remove deprecated comments :)
src/batch/src/executor/insert.rs
Outdated
@@ -119,6 +126,13 @@ impl InsertExecutor { | |||
columns = ordered_cols |
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 there might be a segmentation fault here
ordered_cols[*idx] = columns[i].clone()
if the NULL padding is still not executed here, then the ordered_cols
might not have enough capacity to hold the values at the correct index
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.
Thanks for pointing this out! I believe it's resolved now :)
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. Good work!
|
||
for (idx, expr) in &self.sorted_default_columns { | ||
let column = Column::new(expr.eval(&dummy_chunk).await?); | ||
ordered_columns.push((*idx, column)); |
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 guess the performance won't be too bad if we simply use insert
here, since there won't be too many columns. 😄
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.
Ah that's not mainly about performance but to resolve potential insert index out of range😁
Signed-off-by: Clearlove <yifei.c.wei@gmail.com>
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.
LGTM!
Co-authored-by: Bugen Zhao <i@bugenzhao.com>
Co-authored-by: Bugen Zhao <i@bugenzhao.com>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
insert
given misaligned source is properly handled (following postgres' behavior).Checklist For Contributors
- [ ] I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934)../risedev check
(or alias,./risedev c
)Checklist For Reviewers
Documentation
- [ ] My PR DOES NOT contain user-facing changes.Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note