[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r205250070
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRelFactories.java
##
@@ -122,4 +127,16 @@ public RelNode createJoin(RelNode left, RelNode right,
}
}
+ private static class DrillAggregateFactoryImpl implements
RelFactories.AggregateFactory {
+
+@Override
+public RelNode createAggregate(RelNode input, boolean indicator,
ImmutableBitSet groupSet,
+ ImmutableList groupSets,
List aggCalls) {
+ try {
+return new DrillAggregateRel(input.getCluster(), input.getTraitSet(),
input, indicator, groupSet, groupSets, aggCalls);
+ } catch (InvalidRelException ex) {
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r205250094
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetFilterPushdownWithTransitivePredicates.java
##
@@ -269,5 +269,19 @@ public void testForWithStatementAndDynamicStar() throws
Exception {
final String[] expectedPlan = {"first.*numRowGroups=2",
"second.*numRowGroups=1"};
testPlanMatchingPatterns(query, expectedPlan);
}
+
+ @Test
+ public void testForTransitiveFilterPushPastAgg() throws Exception {
+String query = String.format("SELECT t1.`year` FROM %s t1 WHERE t1.`month`
= 7 and t1.`period` = 2 and t1.`month` IN " +
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r205249991
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillFilterAggregateTransposeRule.java
##
@@ -24,20 +24,29 @@
import org.apache.calcite.rel.core.Filter;
import org.apache.calcite.rel.core.RelFactories;
import org.apache.calcite.rel.rules.FilterAggregateTransposeRule;
+import org.apache.calcite.tools.RelBuilderFactory;
import org.apache.drill.exec.planner.DrillRelBuilder;
public class DrillFilterAggregateTransposeRule extends
FilterAggregateTransposeRule{
// Since Calcite's default FilterAggregateTransposeRule would match Filter
on top of Aggregate, it potentially will match Rels with mixed CONVENTION trait.
- // Here override match method, such that the rule matchs with Rel in the
same CONVENTION.
+ // Here override match method, such that the rule matches with Rel in the
same CONVENTION.
public static final FilterAggregateTransposeRule INSTANCE = new
DrillFilterAggregateTransposeRule();
+ public static final FilterAggregateTransposeRule DRILL_LOGICAL_INSTANCE =
new DrillFilterAggregateTransposeRule(
+ DrillRelBuilder.proto(DrillRelFactories.DRILL_LOGICAL_FILTER_FACTORY,
DrillRelFactories.DRILL_LOGICAL_AGGREGATE_FACTORY),
+ Filter.class, Aggregate.class);
+
private DrillFilterAggregateTransposeRule() {
super(Filter.class,
DrillRelBuilder.proto(Contexts.of(RelFactories.DEFAULT_FILTER_FACTORY)),
Aggregate.class);
}
+ private DrillFilterAggregateTransposeRule(RelBuilderFactory
relBuilderFactory,
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r205250018
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushFilterPastProjectRule.java
##
@@ -40,6 +41,9 @@
public class DrillPushFilterPastProjectRule extends RelOptRule {
public final static RelOptRule INSTANCE = new
DrillPushFilterPastProjectRule(DrillRelFactories.LOGICAL_BUILDER);
+ public final static RelOptRule DRILL_LOGICAL_INSTANCE =
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects URL: https://github.com/apache/drill/pull/1372#discussion_r204952967 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/RuleInstance.java ## @@ -140,4 +145,14 @@ SubQueryRemoveRule SUB_QUERY_JOIN_REMOVE_RULE = new SubQueryRemoveRule.SubQueryJoinRemoveRule(DrillRelFactories.LOGICAL_BUILDER); + + FilterAggregateTransposeRule DRILL_FILTER_AGGREGATE_TRANSPOSE_RULE = Review comment: Done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r204952905
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillPushFilterPastProjectRule.java
##
@@ -50,9 +54,12 @@
}
private DrillPushFilterPastProjectRule(RelBuilderFactory relBuilderFactory) {
-super(operand(LogicalFilter.class, operand(LogicalProject.class, any())),
relBuilderFactory,null);
+super(operand(LogicalFilter.class, operand(LogicalProject.class, any())),
relBuilderFactory, null);
}
+ private DrillPushFilterPastProjectRule(RelBuilderFactory relBuilderFactory,
boolean forDrill) {
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects URL: https://github.com/apache/drill/pull/1372#discussion_r204952377 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/RuleInstance.java ## @@ -140,4 +145,14 @@ SubQueryRemoveRule SUB_QUERY_JOIN_REMOVE_RULE = new SubQueryRemoveRule.SubQueryJoinRemoveRule(DrillRelFactories.LOGICAL_BUILDER); + + FilterAggregateTransposeRule DRILL_FILTER_AGGREGATE_TRANSPOSE_RULE = + new FilterAggregateTransposeRule(Filter.class, + DrillRelBuilder.proto(DrillRelFactories.DRILL_LOGICAL_FILTER_FACTORY, + DrillRelFactories.DRILL_LOGICAL_AGGREGATE_FACTORY), Aggregate.class); + + FilterProjectTransposeRule DRILL_FILTER_PROJECT_TRANSPOSE_RULE = Review comment: Removed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r204951832
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRelFactories.java
##
@@ -122,4 +127,16 @@ public RelNode createJoin(RelNode left, RelNode right,
}
}
+ private static class DrillAggregateFactoryImpl implements
RelFactories.AggregateFactory {
+
+@Override
+public RelNode createAggregate(RelNode input, boolean indicator,
ImmutableBitSet groupSet,
+ ImmutableList groupSets,
List aggCalls) {
+ try {
+return new DrillAggregateRel(input.getCluster(), input.getTraitSet(),
input, indicator, groupSet, groupSets, aggCalls);
+ } catch (InvalidRelException ex) {
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r204951732
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRelFactories.java
##
@@ -122,4 +127,16 @@ public RelNode createJoin(RelNode left, RelNode right,
}
}
+ private static class DrillAggregateFactoryImpl implements
RelFactories.AggregateFactory {
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r204951637
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRelFactories.java
##
@@ -92,7 +97,7 @@ public RelNode createProject(RelNode child,
/**
* Implementation of {@link RelFactories.FilterFactory} that
- * returns a vanilla {@link LogicalFilter}.
+ * returns a vanilla LogicalFilter.
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] gparai commented on a change in pull request #1372: DRILL-6589: Push transitive closure predicates past aggregates/projects
gparai commented on a change in pull request #1372: DRILL-6589: Push transitive
closure predicates past aggregates/projects
URL: https://github.com/apache/drill/pull/1372#discussion_r204951494
##
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestAggregateFunctions.java
##
@@ -732,6 +732,25 @@ public void testPushFilterInExprPastAgg() throws
Exception {
.build().run();
}
+ @Test
+ public void testTransitiveFilterPushPastAgg() throws Exception {
Review comment:
Moved testcase. I decided to remove the push filter past project rule from
TC. It was causing too many side-effects (plan patterns breaking etc.).
Moreover, it may not be very useful from a cost perspective. We can
re-introduce it if it were to be applied in a cost-based manner (via Volcano
planner).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
