zabetak commented on code in PR #5379:
URL: https://github.com/apache/hive/pull/5379#discussion_r1701906142
##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestRuleHelper.java:
##########
@@ -88,16 +83,16 @@ public static RelBuilder
buildRelBuilder(AbstractRelOptPlanner planner,
return HiveRelFactories.HIVE_BUILDER.create(optCluster, schemaMock);
}
- static RexNode eq(RelBuilder relBuilder, String field, Number value) {
+ public static RexNode eq(RelBuilder relBuilder, String field, Number value) {
return relBuilder.call(SqlStdOperatorTable.EQUALS,
relBuilder.field(field), relBuilder.literal(value));
}
- static RexNode or(RelBuilder relBuilder, RexNode... args) {
+ public static RexNode or(RelBuilder relBuilder, RexNode... args) {
return relBuilder.call(SqlStdOperatorTable.OR, args);
}
- static RexNode and(RelBuilder relBuilder, RexNode... args) {
+ public static RexNode and(RelBuilder relBuilder, RexNode... args) {
Review Comment:
The `RelBuilder` API has methods for doing this operations so we don't need
to use these utilities.
##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/TestHiveAugmentMaterializationRule.java:
##########
@@ -22,6 +22,7 @@
import org.apache.hadoop.hive.common.ValidReaderWriteIdList;
import org.apache.hadoop.hive.common.ValidTxnWriteIdList;
import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.ql.optimizer.calcite.TestRuleBase;
Review Comment:
Unrelated changes; do we need them?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRulesRegistry.java:
##########
@@ -27,23 +27,35 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
+import org.apache.calcite.rel.core.Filter;
public class HiveRulesRegistry {
- private SetMultimap<RelOptRule, RelNode> registryVisited;
- private ListMultimap<RelNode, Set<String>> registryPushedPredicates;
+ private final SetMultimap<RelOptRule, String> registryVisited;
+ private final ListMultimap<RelNode, Set<String>> registryPushedPredicates;
public HiveRulesRegistry() {
this.registryVisited = HashMultimap.create();
this.registryPushedPredicates = ArrayListMultimap.create();
}
public void registerVisited(RelOptRule rule, RelNode operator) {
- this.registryVisited.put(rule, operator);
+ this.registryVisited.put(rule, getRelNodeIdentifier(operator));
}
- public Set<RelNode> getVisited(RelOptRule rule) {
- return this.registryVisited.get(rule);
+ public boolean hasBeenVisitedBy(RelOptRule rule, RelNode node) {
+ return this.registryVisited.get(rule).contains(getRelNodeIdentifier(node));
+ }
+
+ private String getRelNodeIdentifier(RelNode node) {
+ if (node instanceof Filter) {
+ String inputClass = ((Filter)
node).getInput().getClass().getSimpleName();
+ String condition = ((Filter) node).getCondition().toString();
+ String rowType = node.getRowType().toString();
Review Comment:
I think there is an edge case where this check can prevent some
optimizations to happen. If you have the same `Filter` in two different places
in the tree (e.g., CTEs) then from now on the `HivePreFilteringRule` will
trigger only on one of those. I am not sure if this should be a show stopper
for merging the PR. I am trying to think of a better way to handle this but
don't have many ideas at the moment.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
