Skip to content

fix: clone data instead of moving it - homemade future is dangerous#3542

Merged
v0y4g3r merged 2 commits intoGreptimeTeam:mainfrom
waynexia:clone-is-fine
Mar 19, 2024
Merged

fix: clone data instead of moving it - homemade future is dangerous#3542
v0y4g3r merged 2 commits intoGreptimeTeam:mainfrom
waynexia:clone-is-fine

Conversation

@waynexia
Copy link
Copy Markdown
Member

I 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?

Fix the change in #3537. Since RecordBatch is all wrapped by Arc, cloning it is not that unacceptable.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Mar 19, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.99%. Comparing base (a99d6eb) to head (cf13f78).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3542      +/-   ##
==========================================
- Coverage   85.37%   84.99%   -0.38%     
==========================================
  Files         909      909              
  Lines      151812   151814       +2     
==========================================
- Hits       129611   129036     -575     
- Misses      22201    22778     +577     

Copy link
Copy Markdown
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, poll master

@v0y4g3r v0y4g3r enabled auto-merge March 19, 2024 13:34
@v0y4g3r v0y4g3r added this pull request to the merge queue Mar 19, 2024
Merged via the queue into GreptimeTeam:main with commit 9816d2a Mar 19, 2024
@tisonkun
Copy link
Copy Markdown
Collaborator

tisonkun commented Mar 19, 2024

Possibly no clone solution -

diff --git a/src/promql/src/extension_plan/series_divide.rs b/src/promql/src/extension_plan/series_divide.rs
index 73fffbdeeb..4bdd3dda56 100644
--- a/src/promql/src/extension_plan/series_divide.rs
+++ b/src/promql/src/extension_plan/series_divide.rs
@@ -255,17 +255,19 @@ impl Stream for SeriesDivideStream {
 
     fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
         loop {
-            if let Some(batch) = self.buffer.take() {
+            if let Some(batch) = self.buffer.as_ref() {
                 let same_length = self.find_first_diff_row(&batch) + 1;
                 if same_length >= batch.num_rows() {
                     let next_batch = match ready!(self.as_mut().fetch_next_batch(cx)) {
                         Some(Ok(next_batch)) => next_batch,
                         None => {
                             self.num_series += 1;
+                            let batch = self.buffer.take().expect("must be some batch");
                             return Poll::Ready(Some(Ok(batch)));
                         }
                         error => return Poll::Ready(error),
                     };
+                    let batch = self.buffer.take().expect("must be some batch");
                     let new_batch = compute::concat_batches(&batch.schema(), &[batch, next_batch])?;
                     self.buffer = Some(new_batch);
                     continue;

... while a new "unchecked" unwrap while the condition should hold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required This change does not impact docs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants