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/01/11 10:08:08 UTC
[GitHub] [calcite] devozerov opened a new pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
devozerov opened a new pull request #2323:
URL: https://github.com/apache/calcite/pull/2323
This PR removes casts to `Logical` operators in `Enumerable` rules.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-757903130
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
devozerov commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-766168047
Looks like the build is failed due to an unrelated flaky test. Is it possible to trigger the rebuild from GitHub? E.g., via a special comment?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#discussion_r554997402
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregateRule.java
##########
@@ -44,7 +45,7 @@ protected EnumerableAggregateRule(Config config) {
}
@Override public @Nullable RelNode convert(RelNode rel) {
- final LogicalAggregate agg = (LogicalAggregate) rel;
+ final Aggregate agg = (Aggregate) rel;
Review comment:
I guess if we apply this patch, all related javadocs should be updated too.
E.g.:
`Rule to convert a {@link LogicalAggregate} to an {@link EnumerableAggregate}.`
=> should be:
`Rule to convert an {@link Aggregate} to an {@link EnumerableAggregate}.`
An similarly in the rest of the impacted classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-766656402
@devozerov If I am not mistaken, closing and re-opening the PR will re-trigger the checks.
PR LGTM, please do not forget (per our [contributing guidelines](https://calcite.apache.org/develop/#contributing)) to squash all commits into a single one and add your name in parentheses at the end of the message
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-767396680
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on a change in pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#discussion_r555036185
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregateRule.java
##########
@@ -44,7 +45,7 @@ protected EnumerableAggregateRule(Config config) {
}
@Override public @Nullable RelNode convert(RelNode rel) {
- final LogicalAggregate agg = (LogicalAggregate) rel;
+ final Aggregate agg = (Aggregate) rel;
Review comment:
Adjusted JavaDoc as requested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-767324711
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-757852039
Looks good, however, I wonder if it is possible to create a test for such cases.
Apparently, trying to instantiate every possible rule with a custom "non-logical" rel is a non-starter as it would require lots of code with no gain.
It might be an interesting case for integration testing like "run Hazelcast (or whatever triggers the case) with trunk Calcite". I believe that might be a way more helpful test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
devozerov commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-757944915
@vlsi regarding testing, I agree that it would be great to have tests here. However, possible options, like manual testing on a per-rule basis, or an additional library, look pretty heavy-weight to me comparing to the scope of changes. Another thing is that some rules already worked with non-Logical classes before this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-766656402
@devozerov If I am not mistaken, closing and re-opening the PR will re-trigger the checks.
PR LGTM, please do not forget (per our [contributing guidelines](https://calcite.apache.org/develop/#contributing)) to squash all commits into a single one and add your name in parentheses at the end of the message
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2323:
URL: https://github.com/apache/calcite/pull/2323#issuecomment-757852989
Another possibility for a test case is ArchUnit with a check like "`enumerable/*` rules must not have casts to `Logical*` types"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia merged pull request #2323: [CALCITE-4461] Do not use `Logical` nodes inside Enumerable rules (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #2323:
URL: https://github.com/apache/calcite/pull/2323
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org