Skip to content
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

[CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule #2646

Closed
wants to merge 2 commits into from

Conversation

mgramin
Copy link

@mgramin mgramin commented Dec 14, 2021

[CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule. Like CalcMergeRule, use only Project, Filter and Calc interfaces, which will expand the range of their applying.

…e. Like CalcMergeRule, use only Project, Filter and Calc interfaces, which will expand the range of their applying.
Copy link
Contributor

@zabetak zabetak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only minor comments left.

General comment independent of this change: Do we really need FilterCalcMergeRule and ProjectCalcMergeRule? They look a bit redundant in the presence of FilterToCalcRule ProjectToCalcRule and CalcMergeRule.

@zabetak zabetak self-assigned this Dec 23, 2021
@mgramin
Copy link
Author

mgramin commented Dec 27, 2021

General comment independent of this change: Do we really need FilterCalcMergeRule and ProjectCalcMergeRule? They look a bit redundant in the presence of FilterToCalcRule ProjectToCalcRule and CalcMergeRule.

FilterCalcMergeRule and ProjectCalcMergeRule produce only LogicalCalc also, it can make further optimizations more difficult.

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Shall we merge it?

@rubenada
Copy link
Contributor

@hsyuan ok for me, but please be aware that we are currently in code freeze since we are in the middle of building a new release. We could merge this patch once master re-opens.

@zabetak
Copy link
Contributor

zabetak commented Apr 11, 2022

I will merge this in a few minutes.

@zabetak zabetak closed this in ef2cc1d Apr 11, 2022
github-actions bot pushed a commit that referenced this pull request Apr 11, 2022
… accept any Filter/Project/Calc operator

Close #2646
github-actions bot pushed a commit to zzxx-husky/calcite that referenced this pull request Apr 11, 2022
aymeric-dispa pushed a commit to aymeric-dispa/calcite that referenced this pull request Jul 23, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 11, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 28, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants