Skip to content
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

chore: table display discard some unused rows #610

Merged
merged 5 commits into from
Mar 28, 2025

Conversation

b41sh
Copy link
Member

@b41sh b41sh commented Mar 27, 2025

  1. display table discards some rows that are not needed because bendsql has a maximum number of rows to display. This can avoid panic caused by excessive data occupying too much memory.
  2. using String instead of &str to convert to Value to avoid unneeded String copying.
  3. upgrade arrow version to 54.0 for compatibe with Databend.

@everpcpc everpcpc requested a review from Copilot March 27, 2025 11:41
Copy link

@Copilot Copilot AI left a 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 refactors the handling of table rows to discard unused rows when displaying results, which helps avoid panics from excessive memory usage, and adjusts value conversions to use owned Strings to prevent unnecessary copying. Key changes include:

  • Updating Value conversion methods to accept and work directly on String.
  • Refactoring row processing in both raw and formatted displays to limit memory usage.
  • Adjusting the session handling to save command history earlier to prevent data loss.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sql/src/value.rs Updates conversion from &str to String and refactors decimal parsing.
sql/src/rows.rs Refactors row construction to preallocate capacity and use zip iteration.
sql/src/raw_rows.rs Similar refactoring for row construction with a slight change in iteration.
cli/src/session.rs Moves history saving before closing the connection to avoid data loss.
cli/src/display.rs Adjusts row counting logic and table creation to discard excess rows.
Comments suppressed due to low confidence (1)

cli/src/display.rs:627

  • [nitpick] The function uses a local variable 'value_rows_count' (computed from results.len()) for determining rendered rows but then uses the parameter 'rows_count' when formatting the summary. Consider renaming and aligning these variables for clarity.
let rows_count_str = format!("{} rows", rows_count);

@sundy-li sundy-li merged commit c66816d into databendlabs:main Mar 28, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants