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

Fix clippy warning about loop that never executes #133

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 12, 2024

When the flightsql feature is enabled clippy is failing with this warning

error: this loop never actually loops
   --> src/app/handlers/flightsql.rs:89:45
    |
89  | / ...                   while let Some(maybe_batch) = stream.next().await {
90  | | ...                       match maybe_batch {
91  | | ...                           Ok(batch) => {
92  | | ...                               info!("Batch rows: {}", batch.num_rows());
...   |
103 | | ...                       }
104 | | ...                   }
    | |_______________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#never_loop
    = note: `#[deny(clippy::never_loop)]` on by default

This code fixes it

@@ -91,14 +91,12 @@ pub fn normal_mode_handler(app: &mut App, key: KeyEvent) {
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

@alamb
Copy link
Contributor Author

alamb commented Sep 12, 2024

I'll try a different approach in this PR

// 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

@matthewmturner matthewmturner merged commit 8185dfe into datafusion-contrib:main Sep 12, 2024
3 checks passed
@alamb alamb deleted the alamb/fix_clippy branch September 12, 2024 13:14
@alamb
Copy link
Contributor Author

alamb commented Sep 12, 2024

🚀

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