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.

Sign up for GitHub

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 clippy warning about loop that never executes #133

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Fix clippy warning about loop that never executes #133

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8a3fba
Fix clippy warning about loop that never executes
alamb Sep 12, 2024
4b1625d
Merge remote-tracking branch 'origin/main' into alamb/fix_clippy
alamb Sep 12, 2024
037826a
Alternate fix for clippy, still only show one batch
alamb Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/app/handlers/flightsql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,25 @@ pub fn normal_mode_handler(app: &mut App, key: KeyEvent) {
match c.do_get(ticket.into_request()).await {
Ok(mut stream) => {
let mut batches: Vec<RecordBatch> = Vec::new();
while let Some(maybe_batch) = stream.next().await {
// temporarily only show the first batch to avoid
// buffering massive result sets. Eventually there should
// be some sort of paging logic
// see https://github.com/datafusion-contrib/datafusion-tui/pull/133#discussion_r1756680874
// while let Some(maybe_batch) = stream.next().await {
if let Some(maybe_batch) = stream.next().await {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes clippy happy by being explicit that only the first batch is needed

match maybe_batch {
Ok(batch) => {
info!("Batch rows: {}", batch.num_rows());
batches.push(batch);
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the complaint is that this break means the outer loop stops

I think the breaks here mean effectively this code would only ever retrieve the first batch from the remote flightsql server which doesn't seem like the actual intent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it actually was the intent but only as a short term hack to prevent queries with large number of results from taking a long time to return result. i still need to implement auto pagination logic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is flightsql and no dataframe to work with i had in mind auto appending LIMIT and OFFSET to the queries. do you have any other ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest:

  1. initially just signal somehow that the stream is truncated and only a part is shown (we can change the code to make this explicit with a comment to get clippy happy)
  2. eventually, I can imagine some much fancier logic that fetches a stream incrementally as the user wants to page through the results (if they do). That would only buffer the rows that haev actually been shown, but would allow the user to scroll through them efficiently

I don't think there is any reason to use LIMIT / OFFSET unless there are a huge number of rows and the offset is large (like OFFSET 1000000 or something). I think handling it on the client side would be a lot easier

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought i had gotten the LIMIT / OFFSET idea from dbeaver but I just double checked and im not seeing it there so yes i agree it doesnt make sense. My original idea was to just paginate through the stream which I believe is same as your second point so I think were aligned on approach here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related dicussion #137

}
Err(e) => {
error!("Error getting batch: {:?}", e);
let elapsed = start.elapsed();
query.set_error(Some(e.to_string()));
query.set_execution_time(elapsed);
break;
}
}
}

let elapsed = start.elapsed();
let rows: usize =
batches.iter().map(|r| r.num_rows()).sum();
Expand Down
Loading