Skip to content

Fix compile and clippy warnings - #52

Merged
NGA-TRAN merged 1 commit into
mainfrom
ntran/cu
Jul 22, 2025
Merged

Fix compile and clippy warnings#52
NGA-TRAN merged 1 commit into
mainfrom
ntran/cu

Conversation

@NGA-TRAN

@NGA-TRAN NGA-TRAN commented Jul 22, 2025

Copy link
Copy Markdown
Collaborator

To avoid compile and clippy warnings, I need to changes these;

  1. Remove unnecessary reference & from stages
  2. Add #[cfg(test)] to import test-only stuffs
  3. Add #[allow(dead_code)] for customizer
  4. Make Status stack Box<Status. heap.
  5. Add #[allow(clippy::result_large_err)] where we do not want t(or cannot) o make Status Box<Status>

Comment thread src/planning.rs
// all stages to workers
let (task_datas, final_addrs) =
assign_to_workers(query_id, &stages, workers.values().collect(), codec)?;
assign_to_workers(query_id, stages, workers.values().collect(), codec)?;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@LiaCastaneda made this &[DDStage] in this https://github.com/datafusion-contrib/datafusion-distributed/pull/48/files and would not need another & herer

Comment thread src/proxy_service.rs
pub fn create_flight_info_response(
&self,
query_plan: QueryPlan,
) -> Result<FlightInfo, Box<Status>> {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Cargo clippy complains this status is over 176 bytes and we want to put this in heap instead

Comment thread src/query_planner.rs
}

pub mod tests {
#[cfg(test)]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Add this to avoid compile warnings that we import wrong places

Comment thread src/worker_service.rs
done: Arc<Mutex<bool>>,

/// Optional customizer for our context and proto serde
#[allow(dead_code)]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I know we use customizer else where but get complain here because it is not used. Put the deadcode here to avoid it.
Also, I wonder if the unit or integration test will help avoid this warning @LiaCastaneda

@LiaCastaneda LiaCastaneda Jul 23, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe I missed something, but do we need to keep the customizer field in DDWorkerHandler? I don't see its being used in DDWorkerHandler, except for creating the PhysicalExtensionCodec , so in theory any customization will be automatically applied/used when we call try_decode on DDCodec 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is used in execute_plan_and_build_stream

if let Some(ref c) = self.customizer {
.

I think we need this to talk with different datasources

Comment thread src/proxy_service.rs
handler: self.handler.clone(),
};

#[allow(clippy::result_large_err)]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Since this function is passed to external FlightServiceServer::with_interceptor that use Status, we cannot change it to Box. So add clippy here to avoid complains

Comment thread src/worker_service.rs

Ok(Box::pin(out_stream))
}
#[allow(clippy::result_large_err)]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ditto. This function implements trait FlightHandler that return Status

@robtandy robtandy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for this cleanup, @NGA-TRAN !

@NGA-TRAN
NGA-TRAN merged commit 07e26de into main Jul 22, 2025
3 checks passed
@NGA-TRAN
NGA-TRAN deleted the ntran/cu branch July 22, 2025 18:28
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.

3 participants