Skip to content

Remove dependency on physical-optimizer on functions-aggregates #14243

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

Closed
alamb opened this issue Jan 22, 2025 · 4 comments · Fixed by #14298
Closed

Remove dependency on physical-optimizer on functions-aggregates #14243

alamb opened this issue Jan 22, 2025 · 4 comments · Fixed by #14298

Comments

@alamb
Copy link
Contributor

alamb commented Jan 22, 2025

          I would like to avoid this dependency if possible -- I will like to make a follow on PR to remove it -- similar to how I did in https://github.com/apache/datafusion/pull/14134

However when I just used the stub a few tests started failing on me

Originally posted by @alamb in #14190 (comment)

@alamb
Copy link
Contributor Author

alamb commented Jan 22, 2025

We are trying to make sure datafusion-physical-optimizers are not dependent on the actual function implementations so ensuring the crate doesn't have dependency will let the compiler ensure this

@berkaysynnada
Copy link
Contributor

I've an idea. You're moving all tests to core/physical_optimizer with #14244. So, as of now, if we are writing tests core/physical_optimizer for physical optimizers, then datafusion/physical_optimizer/test_utils.rs can also be moved to core/physical_optimizer, and that will remove this dep. WDYT? If it sounds good, let's put all others (join_selection, limit-pushdown and more if there are) and test_utils into that PR.

@berkaysynnada
Copy link
Contributor

I've an idea. You're moving all tests to core/physical_optimizer with #14244. So, as of now, if we are writing tests core/physical_optimizer for physical optimizers, then datafusion/physical_optimizer/test_utils.rs can also be moved to core/physical_optimizer, and that will remove this dep. WDYT? If it sounds good, let's put all others (join_selection, limit-pushdown and more if there are) and test_utils into that PR.

I can do that BTW if you are okay, and then work on https://github.com/apache/datafusion/pull/14235/files, and finally delete the core/physical_optimizer.

@alamb
Copy link
Contributor Author

alamb commented Jan 23, 2025

I've an idea. You're moving all tests to core/physical_optimizer with #14244. So, as of now, if we are writing tests core/physical_optimizer for physical optimizers, then datafusion/physical_optimizer/test_utils.rs can also be moved to core/physical_optimizer, and that will remove this dep. WDYT? If it sounds good, let's put all others (join_selection, limit-pushdown and more if there are) and test_utils into that PR.

Yes I think that sounds like a great idea. It would solve this issue too ❤

THANK YOU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants