Append Row to Rows (#4466) #4470
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tustvold
commented
Jun 30, 2023
| @@ -832,14 +871,25 @@ struct RowConfig { | |||
| #[derive(Debug)] | |||
| pub struct Rows { | |||
| /// Underlying row bytes | |||
| buffer: Box<[u8]>, | |||
| buffer: Vec<u8>, | |||
There was a problem hiding this comment.
I need to double-check this doesn't result in a performance regression, it shouldn't but stranger things have happened
alamb
approved these changes
Jun 30, 2023
| Arc::ptr_eq(&row.config.fields, &self.config.fields), | ||
| "row was not produced by this RowConverter" | ||
| ); | ||
| self.config.validate_utf8 |= row.config.validate_utf8; |
There was a problem hiding this comment.
Why doesn't this just assert that the values of validate_utf8 are the same?
There was a problem hiding this comment.
This is consistent with the logic elsewhere, and is ultimately harmless
| /// # use arrow_row::{Row, RowConverter, SortField}; | ||
| /// # use arrow_schema::DataType; | ||
| /// # | ||
| /// let mut converter = RowConverter::new(vec![SortField::new(DataType::Utf8)]).unwrap(); |
There was a problem hiding this comment.
Suggested change
| /// let mut converter = RowConverter::new(vec![SortField::new(DataType::Utf8)]).unwrap(); | |
| /// // This example shows how to buffer only the Row values | |
| /// let mut converter = RowConverter::new(vec![SortField::new(DataType::Utf8)]).unwrap(); |
There was a problem hiding this comment.
This is already stated just above the code block, the commented out imports just make it hard to see
|
Unfortunately this does appear to regress performance, in some cases quite dramatically... |
19 tasks
|
There is a fair bit of noise, but I'm happy this doesn't drastically regress the performance anymore |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #4466
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?