-
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
Push scalar functions into cross join #11528
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.
👍
I think it would be nice to add the reproducer SQL query as a slt, so this won't regress in the future.
The previous behavior was that builtin scalar functions could be pushdown, but UDF couldn't. Now all scalar functions are converted to UDF. |
Marking as draft as the CI is failing and looks like there is feedback to address (thanks @lewiszlw and @simonvandel !) |
@alamb As the behavior changed, I'd like to receive feedback first, then I'll add test and fix ci. |
I think we should pushdown UDF functions into cross join unless there is some reason not to. I suspect the reason they were not being pushed down before was simply an oversight rather than a conscious decision I agree with @simonvandel that ensuring this is covered by tests (if existing tests need to be updated that shows the change is covered). |
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.
Makes sense to me -- thank you @lewiszlw and @simonvandel
cc @liukun4515 / @jackwener in case there is something we are missing about why we can't push filters into CROSS JOIN
s
Which issue does this PR close?
Closes #.
Rationale for this change
DataFusion CLI v37.0.0, pow function can be pushdown into cross join.
DataFusion CLI v40.0.0, pow function can't be pushdown into cross join.
I'm not sure if all UDFs should be pushdown into cross join, open for discussion.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?