[minor]: Introduce IndexSet and IndexMap aliases.

[akurmustafa]

Which issue does this PR close?

Closes #.

Rationale for this change

This Pr is similar to PR. It replaces all usages of indexmap::IndexSet and indexmap::IndexMap with datafusion_common::IndexSet and datafusion_common::IndexMap to enforce consistency across DataFusion.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[jayzhan211]

👍🏻

[findepi]

It replaces all usages of indexmap::IndexSet and indexmap::IndexMap with datafusion_common::IndexSet and datafusion_common::IndexMap to enforce consistency across DataFusion.

if some new code uses indexmap::IndexSet directly, will it cause compile-time or CI failure?

[akurmustafa]

It replaces all usages of indexmap::IndexSet and indexmap::IndexMap with datafusion_common::IndexSet and datafusion_common::IndexMap to enforce consistency across DataFusion.

if some new code uses indexmap::IndexSet directly, will it cause compile-time or CI failure?

Actually, neither as far as I know. This is just a name alias.

[alamb]

Seems like an improvement to me, though as @findepi notes it would be even better with more docs / automatic checking / guidance for developers

[findepi]

Can Clippy's disallowed types or methods be used to enforce consistency?

Otherwise, I would prefer to ensure consistency going into opposite direction.
If datafusion-common doesn't re-export IndexSet, then the code should consistently use indexmap::IndexSet as the only(?) available option.

[findepi]

is indexmap also removed from datafusion/optimizer/Cargo.toml?

[Weijun-H]

I think it cannot be removed because of indexmap::Equivalent.

datafusion/datafusion/optimizer/src/join_key_set.rs

Lines 151 to 155 in 0c01c0e

	implEquivalent<(Expr,Expr)>forExprPair<'_>{
	fnequivalent(&self,other:&(Expr,Expr)) -> bool{
	self.0 == &other.0 && self.1 == &other.1
	}
	}

[alamb]

Let's keep improving on main

[jonahgao]

Can Clippy's disallowed types or methods be used to enforce consistency?

We can add them to clippy disallowed-types.

But I am a bit doubtful whether this PR is necessary because, unlike HashMap, indexmap does not have two implementations in DataFusion that need to be unified. I think indexmap = { workspace = true } already provides consistency.

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 IndexMap.

We also need to create a type alias for indexmap::Equivalent to ensure consistency.

[jonahgao]

I think this should be

pubtypeIndexMap<K,V,S = RandomState> = indexmap::IndexMap<K,V,S>;

[findepi]

I think indexmap = { workspace = true } already provides consistency.

I agree with that and also suggested in #13611 (comment)

In addition, this type alias has added a permanent dependency for datafusion-common

good point.
common is pretty common to depend on, but also is too fat already (eg pulls sqlparser dependency).
avoiding dependencies is a good thing.

[akurmustafa]

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 HashMap and HashSet work. However, it seems that, we have a single option anyway also without enforcing consistency, this is not helpful. Should I retract these changes?

[jonahgao]

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 HashMap and HashSet work. However, it seems that, we have a single option anyway also without enforcing consistency, this is not helpful. Should I retract these changes?

Thanks @akurmustafa. I think retracting these changes will make things simpler.