This is an automated email from the ASF dual-hosted git repository. zabetak pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/hive.git
commit 6b05d64ce8c7161415d97a7896ea50025322e30a Author: Alessandro Solimando <alessandro.solima...@gmail.com> AuthorDate: Mon Oct 17 17:12:18 2022 +0200 HIVE-26643: HiveUnionPullUpConstantsRule produces an invalid plan when pulling up constants for nullable fields (Alessandro Solimando reviewed by Stamatis Zampetakis) Closes #3680 --- .../hadoop/hive/ql/optimizer/calcite/Bug.java | 5 + .../rules/HiveUnionPullUpConstantsRule.java | 27 ++-- .../rules/TestHiveUnionPullUpConstantsRule.java | 178 +++++++++++++++++++++ 3 files changed, 199 insertions(+), 11 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/Bug.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/Bug.java index 32f8cff74b9..91877060ad0 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/Bug.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/Bug.java @@ -78,4 +78,9 @@ public final class Bug { * Whether <a href="https://issues.apache.org/jira/browse/CALCITE-5294">CALCITE-5294</a> is fixed. */ public static final boolean CALCITE_5294_FIXED = false; + + /** + * Whether <a href="https://issues.apache.org/jira/browse/CALCITE-5337">CALCITE-5337</a> is fixed. + */ + public static final boolean CALCITE_5337_FIXED = false; } diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveUnionPullUpConstantsRule.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveUnionPullUpConstantsRule.java index 10d8c1362e7..dcb18072d06 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveUnionPullUpConstantsRule.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveUnionPullUpConstantsRule.java @@ -38,6 +38,7 @@ import org.apache.calcite.tools.RelBuilderFactory; import org.apache.calcite.util.ImmutableBitSet; import org.apache.calcite.util.Pair; import org.apache.calcite.util.mapping.Mappings; +import org.apache.hadoop.hive.ql.optimizer.calcite.Bug; import org.apache.hadoop.hive.ql.optimizer.calcite.HiveRelFactories; import org.apache.hadoop.hive.ql.optimizer.calcite.reloperators.HiveUnion; import org.slf4j.Logger; @@ -66,6 +67,10 @@ public class HiveUnionPullUpConstantsRule extends RelOptRule { @Override public void onMatch(RelOptRuleCall call) { + if (Bug.CALCITE_5337_FIXED) { + throw new IllegalStateException("Class redundant when the fix for CALCITE-5337 is merged into Calcite"); + } + final Union union = call.rel(0); final int count = union.getRowType().getFieldCount(); @@ -82,13 +87,11 @@ public class HiveUnionPullUpConstantsRule extends RelOptRule { return; } - Map<RexNode, RexNode> conditionsExtracted = - RexUtil.predicateConstants(RexNode.class, rexBuilder, predicates.pulledUpPredicates); Map<RexNode, RexNode> constants = new HashMap<>(); for (int i = 0; i < count ; i++) { RexNode expr = rexBuilder.makeInputRef(union, i); - if (conditionsExtracted.containsKey(expr)) { - constants.put(expr, conditionsExtracted.get(expr)); + if (predicates.constantMap.containsKey(expr)) { + constants.put(expr, predicates.constantMap.get(expr)); } } @@ -107,7 +110,11 @@ public class HiveUnionPullUpConstantsRule extends RelOptRule { RexNode expr = rexBuilder.makeInputRef(union, i); RelDataTypeField field = fields.get(i); if (constants.containsKey(expr)) { - topChildExprs.add(constants.get(expr)); + if (constants.get(expr).getType().equals(field.getType())) { + topChildExprs.add(constants.get(expr)); + } else { + topChildExprs.add(rexBuilder.makeCast(field.getType(), constants.get(expr), true)); + } topChildExprsFields.add(field.getName()); } else { topChildExprs.add(expr); @@ -128,16 +135,14 @@ public class HiveUnionPullUpConstantsRule extends RelOptRule { for (int i = 0; i < union.getInputs().size() ; i++) { RelNode input = union.getInput(i); List<Pair<RexNode, String>> newChildExprs = new ArrayList<>(); - for (int j = 0; j < refsIndex.cardinality(); j++ ) { + for (int j = 0; j < refsIndex.cardinality(); j++) { int pos = refsIndex.nth(j); - newChildExprs.add( - Pair.<RexNode, String>of(rexBuilder.makeInputRef(input, pos), - input.getRowType().getFieldList().get(pos).getName())); + newChildExprs.add(Pair.of(rexBuilder.makeInputRef(input, pos), + input.getRowType().getFieldList().get(pos).getName())); } if (newChildExprs.isEmpty()) { // At least a single item in project is required. - newChildExprs.add(Pair.<RexNode,String>of( - topChildExprs.get(0), topChildExprsFields.get(0))); + newChildExprs.add(Pair.of(topChildExprs.get(0), topChildExprsFields.get(0))); } // Add the input with project on top relBuilder.push(input); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestHiveUnionPullUpConstantsRule.java b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestHiveUnionPullUpConstantsRule.java new file mode 100644 index 00000000000..6e455aa732d --- /dev/null +++ b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestHiveUnionPullUpConstantsRule.java @@ -0,0 +1,178 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.optimizer.calcite.rules; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptSchema; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgramBuilder; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.logical.LogicalTableScan; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.tools.RelBuilder; +import org.apache.hadoop.hive.ql.metadata.Table; +import org.apache.hadoop.hive.ql.optimizer.calcite.HiveRelFactories; +import org.apache.hadoop.hive.ql.optimizer.calcite.RelOptHiveTable; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentMatchers; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.Collections; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.lenient; + +@RunWith(MockitoJUnitRunner.class) +public class TestHiveUnionPullUpConstantsRule { + + private final static JavaTypeFactoryImpl JAVA_TYPE_FACTORY = new JavaTypeFactoryImpl(); + + @Mock + private RelOptSchema schemaMock; + @Mock + RelOptHiveTable tableMock; + @Mock + Table hiveTableMDMock; + + private HepPlanner planner; + private RelBuilder rexBuilder; + + private static class MyRecordWithNullableField { + public Integer f1; + public int f2; + public double f3; + } + + private static class MyRecord { + public int f1; + public int f2; + public double f3; + } + + public void before(Class<?> clazz) { + HepProgramBuilder programBuilder = new HepProgramBuilder(); + programBuilder.addRuleInstance(HiveUnionPullUpConstantsRule.INSTANCE); + + planner = new HepPlanner(programBuilder.build()); + + RexBuilder rexBuilder = new RexBuilder(JAVA_TYPE_FACTORY); + final RelOptCluster optCluster = RelOptCluster.create(planner, rexBuilder); + RelDataType rowTypeMock = JAVA_TYPE_FACTORY.createStructType(clazz); + doReturn(rowTypeMock).when(tableMock).getRowType(); + LogicalTableScan tableScan = LogicalTableScan.create(optCluster, tableMock, Collections.emptyList()); + doReturn(tableScan).when(tableMock).toRel(ArgumentMatchers.any()); + doReturn(tableMock).when(schemaMock).getTableForMember(any()); + lenient().doReturn(hiveTableMDMock).when(tableMock).getHiveTableMD(); + + this.rexBuilder = HiveRelFactories.HIVE_BUILDER.create(optCluster, schemaMock); + } + + public RexNode eq(String field, Number value) { + return rexBuilder.call(SqlStdOperatorTable.EQUALS, + rexBuilder.field(field), rexBuilder.literal(value)); + } + + private void test(RelNode plan, String expectedPrePlan, String expectedPostPlan) { + planner.setRoot(plan); + RelNode optimizedRelNode = planner.findBestExp(); + assertEquals("Original plans do not match", expectedPrePlan, RelOptUtil.toString(plan)); + assertEquals("Optimized plans do not match", expectedPostPlan, RelOptUtil.toString(optimizedRelNode)); + } + + @Test + public void testNonNullableFields() { + before(MyRecord.class); + + final RelNode plan = rexBuilder + .scan("t") + .filter(eq("f1",1)) + .project(rexBuilder.field("f1"), rexBuilder.field("f2")) + .scan("t") + .filter(eq("f1",1)) + .project(rexBuilder.field("f1"), rexBuilder.field("f2")) + .union(true) + .build(); + + String prePlan = "HiveUnion(all=[true])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n"; + + String postPlan = "HiveProject(f1=[1], f2=[$0])\n" + + " HiveUnion(all=[true])\n" + + " HiveProject(f2=[$1])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n" + + " HiveProject(f2=[$1])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n"; + + test(plan, prePlan, postPlan); + } + + @Test + public void testNullableFields() { + before(MyRecordWithNullableField.class); + + final RelNode plan = rexBuilder + .scan("t") + .filter(eq("f1",1)) + .project(rexBuilder.field("f1"), rexBuilder.field("f2")) + .scan("t") + .filter(eq("f1",1)) + .project(rexBuilder.field("f1"), rexBuilder.field("f2")) + .union(false) + .build(); + + String prePlan = "HiveUnion(all=[true])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n"; + + String postPlan = "HiveProject(f1=[CAST(1):JavaType(class java.lang.Integer)], f2=[$0])\n" + + " HiveUnion(all=[true])\n" + + " HiveProject(f2=[$1])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n" + + " HiveProject(f2=[$1])\n" + + " HiveProject(f1=[$0], f2=[$1])\n" + + " HiveFilter(condition=[=($0, 1)])\n" + + " LogicalTableScan(table=[[]])\n"; + + test(plan, prePlan, postPlan); + } +}