-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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]: Introduce IndexSet and IndexMap aliases. #13611
Conversation
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.
👍🏻
if some new code uses |
Actually, neither as far as I know. This is just a name alias. |
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.
Seems like an improvement to me, though as @findepi notes it would be even better with more docs / automatic checking / guidance for developers
Can Clippy's disallowed types or methods be used to enforce consistency? Otherwise, I would prefer to ensure consistency going into opposite direction. |
use datafusion_expr::logical_plan::LogicalPlan; | ||
use datafusion_expr::{Aggregate, Expr, Sort, SortExpr}; | ||
use indexmap::IndexSet; |
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.
is indexmap
also removed from datafusion/optimizer/Cargo.toml
?
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.
I think it cannot be removed because of indexmap::Equivalent
.
datafusion/datafusion/optimizer/src/join_key_set.rs
Lines 151 to 155 in 0c01c0e
impl Equivalent<(Expr, Expr)> for ExprPair<'_> { | |
fn equivalent(&self, other: &(Expr, Expr)) -> bool { | |
self.0 == &other.0 && self.1 == &other.1 | |
} | |
} |
Let's keep improving on main |
We can add them to clippy disallowed-types. But I am a bit doubtful whether this PR is necessary because, unlike In addition, this type alias has added a permanent dependency for datafusion-common and can propagate to other packages. Even if one day, common/cse.rs no longer needs We also need to create a type alias for indexmap::Equivalent to ensure consistency. |
@@ -93,6 +94,9 @@ pub use error::{ | |||
pub type HashMap<K, V, S = DefaultHashBuilder> = hashbrown::HashMap<K, V, S>; | |||
pub type HashSet<T, S = DefaultHashBuilder> = hashbrown::HashSet<T, S>; | |||
|
|||
pub type IndexMap<T, S = DefaultHashBuilder> = indexmap::IndexMap<T, S>; |
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.
I think this should be
pub type IndexMap<K, V, S = RandomState> = indexmap::IndexMap<K, V, S>;
I agree with that and also suggested in #13611 (comment)
good point. |
I aggree with @findepi and @jonahgao that this might indeed be unnecessary. At first, this occurred to me as a natural continuation of the previous |
Thanks @akurmustafa. I think retracting these changes will make things simpler. |
Which issue does this PR close?
Closes #.
Rationale for this change
This Pr is similar to PR. It replaces all usages of
indexmap::IndexSet
andindexmap::IndexMap
withdatafusion_common::IndexSet
anddatafusion_common::IndexMap
to enforce consistency across DataFusion.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?