You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/12/14 13:36:50 UTC
[GitHub] [calcite] mgramin opened a new pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
mgramin opened a new pull request #2646:
URL: https://github.com/apache/calcite/pull/2646
[CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule. Like CalcMergeRule, use only Project, Filter and Calc interfaces, which will expand the range of their applying.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mgramin commented on pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
Posted by GitBox <gi...@apache.org>.
mgramin commented on pull request #2646:
URL: https://github.com/apache/calcite/pull/2646#issuecomment-1001489758
> 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.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2646:
URL: https://github.com/apache/calcite/pull/2646#issuecomment-1066500767
@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.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2646:
URL: https://github.com/apache/calcite/pull/2646#discussion_r774665101
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ProjectCalcMergeRule.java
##########
@@ -82,8 +82,6 @@ public ProjectCalcMergeRule(RelBuilderFactory relBuilderFactory) {
project.getRowType(),
cluster.getRexBuilder());
if (RexOver.containsOver(program)) {
- LogicalCalc projectAsCalc = LogicalCalc.create(calc, program);
Review comment:
This is a small breaking change. I doubt people will notice but let's put a small note in `history.md` to be on the safe side. Moreover, please remove the now obsolete comments in lines 73-75 above.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] mgramin commented on a change in pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
Posted by GitBox <gi...@apache.org>.
mgramin commented on a change in pull request #2646:
URL: https://github.com/apache/calcite/pull/2646#discussion_r769602846
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ProjectCalcMergeRule.java
##########
@@ -82,8 +82,6 @@ public ProjectCalcMergeRule(RelBuilderFactory relBuilderFactory) {
project.getRowType(),
cluster.getRexBuilder());
if (RexOver.containsOver(program)) {
- LogicalCalc projectAsCalc = LogicalCalc.create(calc, program);
Review comment:
Unfortunately, yes. But i assume that special rule `ProjectToCalcRule` can perform simple conversion from `LogicalProject` to `LogicalCalc`. Whereas `ProjectCalcMergeRule` will be able to stay more clear and more generalize.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2646: [CALCITE-4936] Generalize FilterCalcMergeRule and ProjectCalcMergeRule
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2646:
URL: https://github.com/apache/calcite/pull/2646#discussion_r769007766
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ProjectCalcMergeRule.java
##########
@@ -82,8 +82,6 @@ public ProjectCalcMergeRule(RelBuilderFactory relBuilderFactory) {
project.getRowType(),
cluster.getRexBuilder());
if (RexOver.containsOver(program)) {
- LogicalCalc projectAsCalc = LogicalCalc.create(calc, program);
Review comment:
Isn't this a breaking change? Currently the Project is removed, with this proposal the Project remains.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org