zhangfengcdt commented on code in PR #565:
URL: https://github.com/apache/sedona-db/pull/565#discussion_r2783386223
##########
rust/sedona/src/show.rs:
##########
@@ -521,7 +533,12 @@ impl DisplayColumn {
/// Create a cell for this column. This is where alignment for numeric
/// output is applied.
fn cell<T: ToString>(&self, content: T) -> Cell {
- let cell = Cell::new(content).set_delimiter('\0');
+ // 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();
Review Comment:
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?
##########
rust/sedona/src/show.rs:
##########
@@ -316,29 +322,32 @@ impl<'a> DisplayTable<'a> {
/// number of characters in other columns (based on user options).
fn minimum_width(&self) -> Result<u16> {
// Leftmost border
- let mut total_size = 1;
- let mut hidden_count = 0;
+ let mut total_size: u16 = 1;
+ let mut hidden_count: u16 = 0;
for col in &self.columns {
// Padding on both sides plus separator
- total_size += 3;
+ total_size = total_size.saturating_add(3);
Review Comment:
Not very clear to me why "3" here? Maybe define a constant or add comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
