-
Notifications
You must be signed in to change notification settings - Fork 855
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: IPC writer should truncate string array with all empty string #2314
Conversation
@viirya please review |
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.
Does it cause a failure?
// actual
assert_eq!(serialize(&record_batch).len(), serialize(&record_batch_slice).len());
It just doesn't truncate the record batch, right?
"It just doesn't truncate the record batch, right?" @viirya |
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.
Looks correct. Thanks for fixing it.
It’s my pleasure @viirya |
Codecov Report
@@ Coverage Diff @@
## master #2314 +/- ##
==========================================
- Coverage 81.26% 81.25% -0.01%
==========================================
Files 248 248
Lines 60605 60623 +18
==========================================
+ Hits 49249 49258 +9
- Misses 11356 11365 +9
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Benchmark runs are scheduled for baseline = e835853 and contender = d56d88e. d56d88e 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?
Closes #2312 (comment)
Rationale for this change
fixed ipc truncation bug
What changes are included in this PR?
Are there any user-facing changes?