-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Minor: make LeftJoinData into a struct in CrossJoinExec #13227
Conversation
@@ -47,7 +47,15 @@ use async_trait::async_trait; | |||
use futures::{ready, Stream, StreamExt, TryStreamExt}; | |||
|
|||
/// Data of the left side | |||
type JoinLeftData = (RecordBatch, MemoryReservation); | |||
#[derive(Debug)] | |||
struct JoinLeftData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the PR is to name this tuple and add comments
#[allow(dead_code)] | ||
reservation: MemoryReservation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this can be also written without disabling a lint:
#[allow(dead_code)] | |
reservation: MemoryReservation, | |
_reservation: MemoryReservation, |
#[allow(dead_code)]
has disadvantage that it can be retained in the code even if the field becomes used_reservation
has disadvantage that it doesn't look pretty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty or not, _reservation
is the language-appropriate idiom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #13278 to track. Thank you for the suggestion
Which issue does this PR close?
Closes #.
Rationale for this change
I was in this code for #13223 and #13203 and noticed the use of the unnamed tuple
By giving the fields names I think it becomes easier to read and comment, as well as more consistent with the other joins (like HashJoinExec):
datafusion/datafusion/physical-plan/src/joins/hash_join.rs
Lines 82 to 83 in 2047d7f
What changes are included in this PR?
Change from a tuple to a named struct
Are these changes tested?
By existing CI
Are there any user-facing changes?