alamb commented on code in PR #11798: URL: https://github.com/apache/datafusion/pull/11798#discussion_r1703161610
########## datafusion/core/src/physical_optimizer/tests/aggregate_statistics.rs: ########## @@ -15,292 +15,19 @@ // specific language governing permissions and limitations // under the License. -//! Utilizing exact statistics from sources to avoid scanning data Review Comment: I worry that putting these tests in `datafusion/core/src/physical_optimizer/tests/aggregate_statistics.rs` will make them harder to find becase: 1. It is in a different crate than being tested 2. It is in a non standard testing location As I understand it, these tests use code in other modules that physical optimizer doesn't depend on (like `functions-aggregate` Thus, I would like to suggest we put these tests https://github.com/apache/datafusion/tree/main/datafusion/core/tests Perhaps something like `physical_optimizer_integration.rs` to mirror https://github.com/apache/datafusion/blob/main/datafusion/core/tests/optimizer_integration.rs ########## datafusion/physical-optimizer/Cargo.toml: ########## @@ -34,5 +34,6 @@ workspace = true [dependencies] datafusion-common = { workspace = true, default-features = true } datafusion-execution = { workspace = true } +datafusion-expr = { workspace = true } Review Comment: 🤔 Why is this needed? If it is only for ```rust use datafusion_expr::utils::COUNT_STAR_EXPANSION; ``` I think we could potentially move `COUNT_STAR_EXPANSION` to `datafusion_common` (and leave a backwards compatible `pub use`) and avoid this dependency I think in general we are trying to keep the division between logical and physical exprs separate when possible -- this makes it easier for systems that only need physical plans (e.g. DataFusion Comet) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
