-
Notifications
You must be signed in to change notification settings - Fork 43
fix(rust/sedona): Fix panic when displaying very long content #565
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
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.
Pull request overview
This PR fixes a panic that occurred when displaying tables with unexpectedly large content, particularly in explain plans. The fix addresses unchecked arithmetic operations that could cause integer overflow by introducing saturating addition and conservative maximum width limits.
Changes:
- Introduced
WIDTH_MAXconstant (32,000) to prevent overflow in comfy table's internal arithmetic - Replaced unchecked addition with
saturating_addthroughout width calculations - Added content truncation in the
cellmethod to limit string length toWIDTH_MAX
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
| // Never return content longer than WIDTH_MAX because comfy table may | ||
| // do arithmetic with it without checking for overflow. | ||
| let content_str = content.to_string(); | ||
| let content_safe: String = content_str.chars().take(WIDTH_MAX as usize).collect(); |
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.
This seems to be limiting the character length, but not display length (width). They may not be the same or may have issues with multi-byte characters. Maybe use unicode width string handling?
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.
These are uniocde widths! https://doc.rust-lang.org/std/primitive.str.html#method.chars . (We also have tests for unicode output to ensure the character math works for that).
| for col in &self.columns { | ||
| // Padding on both sides plus separator | ||
| total_size += 3; | ||
| total_size = total_size.saturating_add(3); |
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.
Not very clear to me why "3" here? Maybe define a constant or add comments?
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.
There is a comment // Padding on both sides plus separator but I am happy to clarify in a follow-up if needed!
Fixes a panic when the content of a table is unexpectedly large. The first issue I hit this was was explain plans.
In addition to at least one issue here, there is quite a lot of unchecked addition in the underlying renderer, which we work around by truncating any u16s or strings to 32,000.
Reprex from original issue now renders without panicing:
Closes #388.