refactor: pull column filling logic out of mito worker loop #5455
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/mito2/src/worker/handle_write.rs:237
- Cloning
region_metadatacan be optimized to avoid unnecessary cloning.
let region_metadata = self.get_metadata(region_id).ok();
src/mito2/src/worker.rs:696
- The type should be Vec instead of RequestBuffer.
let mut general_req_buffer: Vec<WorkerRequest> = RequestBuffer::with_capacity(self.config.worker_request_batch_size);
| @@ -875,7 +917,7 @@ mod tests { | |||
| rows: vec![], | |||
| }; | |||
|
|
|||
| let err = WriteRequest::new(RegionId::new(1, 1), OpType::Put, rows).unwrap_err(); | |||
There was a problem hiding this comment.
nitpick: Add a unit test that the metadata is not None.
| for row in &mut self.rows.rows { | ||
| row.values.push(proto_value.clone()); | ||
| row.values.extend(default_values.clone()); |
There was a problem hiding this comment.
non-blocking: Maybe we can only clone the items.
| row.values.extend(default_values.clone()); | |
| row.values.extend(default_values.iter().cloned()); |
| @@ -66,13 +66,20 @@ pub struct WriteRequest { | |||
| has_null: Vec<bool>, | |||
| /// Write hint. | |||
| pub hint: Option<WriteHint>, | |||
| /// Region metadata on the time of this request is created. | |||
| pub region_metadata: Option<RegionMetadataRef>, | |||
There was a problem hiding this comment.
non-blocking: maybe pub(crate).
Signed-off-by: Ruihang Xia waynestxia@gmail.comI hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
As title says. Also with some other changes that reduce some overhead.
We can further pull more logic out of the worker, like primary key encoding or row to column conversion.
PR Checklist
Please convert it to a draft if some of the following conditions are not met.