chore: move SanityChecker into physical-optimizer crate

[mnpw]

Which issue does this PR close?

Closes #14072.

Rationale for this change

From #14072

Historically DataFusion was one (very) large crate datafusion, and as it grew bigger we extracted various functionality into separate crates. This leads to both faster compile times (as the crates can be compiled in parallel) as well easier to navigate code (as the crates force a cleaner dependency separation)

What changes are included in this PR?

• Move SanityChecker from datafusion crate to datafusion-physical-optimizer crate

Are these changes tested?

Ran cargo test

Are there any user-facing changes?

No

[mnpw]

@alamb for your consideration.

I don't like that datafusion-physical-optimizer needs to use datafusion crate as a dev-dependency. This was required as SanityChecker tests were tightly coupled with the helper functions in datafusion::physical_optimizer::test_utils. Perhaps we can consider moving these helper functions somewhere outside, say, to test-utils crate?

[alamb]

I don't like that datafusion-physical-optimizer needs to use datafusion crate as a dev-dependency.

Thanks @mnpw -- I agree this is not good and we shouldn't merge it in like that (and in fact I think it failed the circular dependency check)

I think one of the major sources of this dependency is the use of ParquetExec which is still defined in the core module...

Since datafusion already depends on phsyical-optimizer, what would you think about moving as many of the physical optimizer utils from

• https://github.com/apache/datafusion/blob/main/datafusion/core/src/physical_optimizer/test_utils.rs
  To
• https://github.com/apache/datafusion/tree/main/datafusion/physical-optimizer/src/test_utils.rs
  As possible?

[alamb]

Converting to draft given the dependency situation we have uncovered

[mnpw]

@alamb

Here's my understanding of the issue:

• datafusion crate already depends on datafusion-physical-optimizer.
• SanityChecker's tests depend on datafusion::datasources::{listing, physical_plan, stream} (via datafusion::physical_optimizer::test_utils).
• Moving SanityChecker, with its tests, to datafusion-physical-optimizer makes datafusion-physical-optimizer depend on datafusion::datasources.
• This creates a circular dependency between datafusion and datafusion-physical-optimizer.

In this case, best solution seems to be to move SanityChecker to datafusion-physical-optimizer while keeping its tests in datafusion/core/tests.

I have re-worked the PR and made the following changes:

• Move SanityChecker to datafusion-physical-optimizer
• Move SanityChecker's tests to datafusion/core/tests
• Move independent componenets from datafusion::physical_optimizer::test_utils to datafusion-physical-optimizer::test_utils

[alamb]

I think we are trying very hard to avoid this dependency -- specifically we are trying to avoid having the optimizer depend on any actual function implementations

The separation ensures that there is no special treatment for provided functions vs built in ones

I played around with this some morning and I found a way to remove it here:

• Remove dependency on physical-optimizer on functions-aggregates #14134

[alamb]

Thanks @mnpw -- this looks great. I had one improvement but we can do this as a follow on PR

[alamb]

I will update the datafusion-cli cargo file as well

[mnpw]

Thanks @alamb for proactively reviewing and adding the fix commits. Much appreciated 🙏

[alamb]

Thanks @alamb for proactively reviewing and adding the fix commits. Much appreciated 🙏

Thank you for helping push this along ❤️