zabetak commented on code in PR #6720:
URL: https://github.com/apache/hive/pull/6720#discussion_r3880411487
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelJson.java:
##########
Review Comment:
Not blocking for the PR but if we can test and get rid of the entire class
as part of this cleanup it would be great.
##########
data/conf/hive-log4j2.properties:
##########
@@ -114,7 +114,7 @@ logger.CalcitePlanner.name =
org.apache.calcite.plan.RelOptPlanner
logger.CalcitePlanner.level = INFO
# Change logger level to DEBUG, to see all application of CBO rules and the
operators they produce
-logger.CBORuleLogger.name =
org.apache.hadoop.hive.ql.optimizer.calcite.RuleEventLogger
+logger.CBORuleLogger.name = org.apache.calcite.plan.RuleEventLogger
Review Comment:
Did you verify that the logs are displayed as expected after the change?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRemoveEmptySingleRules.java:
##########
@@ -270,85 +114,40 @@ public void onMatch(RelOptRuleCall call) {
.toRule();
public static final RelOptRule UNION_INSTANCE =
- new HiveUnionEmptyPruneRuleConfig()
+ new UnionEmptyPruneRuleConfig()
.withOperandSupplier(b0 ->
b0.operand(HiveUnion.class).unorderedInputs(b1 ->
b1.operand(Values.class)
.predicate(Values::isEmpty).noInputs()))
.withDescription("HivePruneEmptyUnionBranch")
.toRule();
- /**
- * Copy of {@link PruneEmptyRules.UnionEmptyPruneRuleConfig} but this
version expects {@link Union}.
- */
- private static final class HiveUnionEmptyPruneRuleConfig extends
HiveRuleConfig implements PruneEmptyRules.PruneEmptyRule.Config {
- @Override
- public PruneEmptyRules.PruneEmptyRule toRule() {
- return new PruneEmptyRules.PruneEmptyRule(this) {
- @Override public void onMatch(RelOptRuleCall call) {
- if (Bug.CALCITE_5293_FIXED) {
- throw new IllegalStateException(
- "Class HiveUnionEmptyPruneRuleConfig is redundant after
fix is merged into Calcite");
- }
+ private static final class RemoveEmptySingleRuleConfig extends HiveRuleConfig
+ implements RemoveEmptySingleRule.RemoveEmptySingleRuleConfig {
+ }
- final Union union = call.rel(0);
- final List<RelNode> inputs = union.getInputs();
- assert inputs != null;
- final RelBuilder builder = call.builder();
- int nonEmptyInputs = 0;
- for (RelNode input : inputs) {
- if (!isEmpty(input)) {
- builder.push(input);
- nonEmptyInputs++;
- }
- }
- assert nonEmptyInputs < inputs.size()
- : "planner promised us at least one Empty child: "
- + RelOptUtil.toString(union);
- if (nonEmptyInputs == 0) {
- builder.push(union).empty();
- } else {
- builder.union(union.all, nonEmptyInputs);
- builder.convert(union.getRowType(), true);
- }
- call.transformTo(builder.build());
- }
- };
- }
+ private static final class SortFetchZeroRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.SortFetchZeroRuleConfig {
}
- private static boolean isEmpty(RelNode node) {
- if (Bug.CALCITE_5293_FIXED) {
- throw new IllegalStateException(
- "Method HiveRemoveEmptySingleRules.isEmpty is redundant after
fix is merged into Calcite");
- }
+ private static final class UnionEmptyPruneRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.UnionEmptyPruneRuleConfig {
+ }
- if (node instanceof Values) {
- return ((Values) node).getTuples().isEmpty();
- }
- if (node instanceof HepRelVertex) {
- return isEmpty(((HepRelVertex) node).getCurrentRel());
- }
- // Note: relation input might be a RelSubset, so we just iterate over the
relations
- // in order to check if the subset is equivalent to an empty relation.
- if (!(node instanceof RelSubset)) {
- return false;
- }
- RelSubset subset = (RelSubset) node;
- for (RelNode rel : subset.getRels()) {
- if (isEmpty(rel)) {
- return true;
- }
- }
- return false;
+ private static final class JoinLeftEmptyRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.JoinLeftEmptyRuleConfig {
}
Review Comment:
Can we do a bit better in terms of cleanup and drop this kind of trivial
extension class? Maybe we can instantiate the rules directly using the configs
available in Calcite. For example:
```
public static final RelOptRule JOIN_LEFT_INSTANCE =
PruneEmptyRules.JoinLeftEmptyRuleConfig.DEFAULT
.withRelBuilderFactory(HiveRelFactories.HIVE_BUILDER).toRule();
public static final RelOptRule JOIN_RIGHT_INSTANCE =
PruneEmptyRules.JoinRightEmptyRuleConfig.DEFAULT
.withRelBuilderFactory(HiveRelFactories.HIVE_BUILDER).toRule();
```
Maybe this allow us to drop also some SEMI/ANTI rule instances. Can you
check if its feasible?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRemoveEmptySingleRules.java:
##########
@@ -270,85 +114,40 @@ public void onMatch(RelOptRuleCall call) {
.toRule();
public static final RelOptRule UNION_INSTANCE =
- new HiveUnionEmptyPruneRuleConfig()
+ new UnionEmptyPruneRuleConfig()
.withOperandSupplier(b0 ->
b0.operand(HiveUnion.class).unorderedInputs(b1 ->
b1.operand(Values.class)
.predicate(Values::isEmpty).noInputs()))
.withDescription("HivePruneEmptyUnionBranch")
.toRule();
- /**
- * Copy of {@link PruneEmptyRules.UnionEmptyPruneRuleConfig} but this
version expects {@link Union}.
- */
- private static final class HiveUnionEmptyPruneRuleConfig extends
HiveRuleConfig implements PruneEmptyRules.PruneEmptyRule.Config {
- @Override
- public PruneEmptyRules.PruneEmptyRule toRule() {
- return new PruneEmptyRules.PruneEmptyRule(this) {
- @Override public void onMatch(RelOptRuleCall call) {
- if (Bug.CALCITE_5293_FIXED) {
- throw new IllegalStateException(
- "Class HiveUnionEmptyPruneRuleConfig is redundant after
fix is merged into Calcite");
- }
+ private static final class RemoveEmptySingleRuleConfig extends HiveRuleConfig
+ implements RemoveEmptySingleRule.RemoveEmptySingleRuleConfig {
+ }
- final Union union = call.rel(0);
- final List<RelNode> inputs = union.getInputs();
- assert inputs != null;
- final RelBuilder builder = call.builder();
- int nonEmptyInputs = 0;
- for (RelNode input : inputs) {
- if (!isEmpty(input)) {
- builder.push(input);
- nonEmptyInputs++;
- }
- }
- assert nonEmptyInputs < inputs.size()
- : "planner promised us at least one Empty child: "
- + RelOptUtil.toString(union);
- if (nonEmptyInputs == 0) {
- builder.push(union).empty();
- } else {
- builder.union(union.all, nonEmptyInputs);
- builder.convert(union.getRowType(), true);
- }
- call.transformTo(builder.build());
- }
- };
- }
+ private static final class SortFetchZeroRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.SortFetchZeroRuleConfig {
}
- private static boolean isEmpty(RelNode node) {
- if (Bug.CALCITE_5293_FIXED) {
- throw new IllegalStateException(
- "Method HiveRemoveEmptySingleRules.isEmpty is redundant after
fix is merged into Calcite");
- }
+ private static final class UnionEmptyPruneRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.UnionEmptyPruneRuleConfig {
+ }
- if (node instanceof Values) {
- return ((Values) node).getTuples().isEmpty();
- }
- if (node instanceof HepRelVertex) {
- return isEmpty(((HepRelVertex) node).getCurrentRel());
- }
- // Note: relation input might be a RelSubset, so we just iterate over the
relations
- // in order to check if the subset is equivalent to an empty relation.
- if (!(node instanceof RelSubset)) {
- return false;
- }
- RelSubset subset = (RelSubset) node;
- for (RelNode rel : subset.getRels()) {
- if (isEmpty(rel)) {
- return true;
- }
- }
- return false;
+ private static final class JoinLeftEmptyRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.JoinLeftEmptyRuleConfig {
}
- private static final class RemoveEmptySingleRuleConfig extends HiveRuleConfig
- implements RemoveEmptySingleRule.RemoveEmptySingleRuleConfig {
+ private static final class JoinRightEmptyRuleConfig extends HiveRuleConfig
+ implements PruneEmptyRules.JoinRightEmptyRuleConfig {
}
- private static final class SortFetchZeroRuleConfig extends HiveRuleConfig
- implements PruneEmptyRules.SortFetchZeroRuleConfig {
+ private static final class CorrelateLeftEmptyRuleConfig extends
HiveRuleConfig
+ implements PruneEmptyRules.CorrelateLeftEmptyRuleConfig {
+ }
Review Comment:
Likewise let's re-evaluate how much we need the trivial extension classes.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveTypeFactory.java:
##########
Review Comment:
How about setting `calcite.default.charset` property in saffron and get rid
of the entire class?
--
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]
