-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(MemTable): make it cancel-safe and fix parallelism #5197
Conversation
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 @DDtKey ❤️
I left a suggestion that I think would make it even better but I think this is an improvement (internal error vs panic 👍 ) already and could go in as is
data.push(result.map_err(|_| { | ||
DataFusionError::Internal( | ||
"MemTable::load could not join task".to_string(), | ||
) |
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 it would help if we could preserve the original error here rather than turning it into an internal error
Perhaps using DataFusionError::Context
data.push(result.map_err(|_| { | |
DataFusionError::Internal( | |
"MemTable::load could not join task".to_string(), | |
) | |
data.push(result.map_err(|e| { | |
DataFusionError::Context( | |
"MemTable::load could not join task".to_string(), Box::new(e) | |
) |
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.
Context
requires DataFusionError
, so it's anyway should be wrapped to something 🤔
Could be data.push(result.map_err(|e| DataFusionError::External(Box::new(e)))??)
, WDYT?
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.
That would definitely be better than 👍
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.
Done 🙂
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 again @DDtKey
Benchmark runs are scheduled for baseline = 816a0f8 and contender = f63b972. f63b972 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
The issue is close to #5178, but for
MemTable
.Rationale for this change
await
was sequentialWhat changes are included in this PR?
Are these changes tested?
Existed tests are passing
Are there any user-facing changes?