zabetak commented on code in PR #6895: URL: https://github.com/apache/hive/pull/6895#discussion_r4347166094
########## ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/translator/TestExprNodeConverter.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.translator; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.hadoop.hive.ql.optimizer.calcite.HiveTypeSystemImpl; +import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; +import org.junit.Test; + +import java.math.BigDecimal; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +public class TestExprNodeConverter { + + private final RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl(new HiveTypeSystemImpl()); + private final RexBuilder rexBuilder = new RexBuilder(typeFactory); + private final RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + private final RelDataType rowType = typeFactory.builder().add("a", intType).add("b", intType).build(); + + private ExprNodeDesc convert(RexNode node) { + return node.accept(new ExprNodeConverter("t", rowType, Collections.emptySet(), typeFactory)); + } + + private RexNode intLiteral(int value) { + return rexBuilder.makeExactLiteral(BigDecimal.valueOf(value), intType); + } + + private RexNode equalsCall(RexNode left, RexNode right) { + return rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, left, right); + } + + @Test + public void testAndWithDuplicateConstantOperandsIsUnwrapped() { + RexNode eq = equalsCall(intLiteral(1), intLiteral(1)); + ExprNodeDesc result = convert(rexBuilder.makeCall(SqlStdOperatorTable.AND, eq, eq)); + + List<ExprNodeDesc> resultNodes = result.getChildren(); + assertEquals(2, resultNodes.size()); + assertEquals("Const int 1", resultNodes.get(0).toString()); + assertEquals("Const int 1", resultNodes.get(1).toString()); Review Comment: Why not simply assert on the result? ```suggestion assertEquals("GenericUDFOPEqual(Const int 1, Const int 1)", result.toString()); ``` All the structure is clearly visible in the string representation. Same for the other test cases. ########## ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestHiveRexExecutor.java: ########## @@ -0,0 +1,76 @@ +/* + * 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; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlBinaryOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.Test; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class TestHiveRexExecutor { + + private final RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl(new HiveTypeSystemImpl()); + private final RexBuilder rexBuilder = new RexBuilder(typeFactory); + private final RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + + private RexNode intLiteral(int value) { + return rexBuilder.makeExactLiteral(BigDecimal.valueOf(value), intType); + } + + private RexNode equalsCall(RexNode left, RexNode right) { + return rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, left, right); + } + + @Test + public void testReduceAndWithDuplicateConstantOperands() { + assertSpecifiedOperatorWithDuplicateConstantOperands(SqlStdOperatorTable.AND); + } + + @Test + public void testReduceOrWithDuplicateConstantOperands() { + assertSpecifiedOperatorWithDuplicateConstantOperands(SqlStdOperatorTable.OR); + } + + private void assertSpecifiedOperatorWithDuplicateConstantOperands(SqlBinaryOperator operator) { + RexNode eq = equalsCall(intLiteral(1), intLiteral(1)); + RexNode and = rexBuilder.makeCall(operator, eq, eq); + List<RexNode> reduced = new ArrayList<>(); + + new HiveRexExecutorImpl().reduce(rexBuilder, Collections.singletonList(and), reduced); + + assertEquals(1, reduced.size()); + assertTrue(reduced.get(0) instanceof RexLiteral); + assertEquals(Boolean.TRUE, ((RexLiteral) reduced.get(0)).getValueAs(Boolean.class)); Review Comment: Again assertion on the string representation of the result would be easier to read. ########## ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ExprNodeConverter.java: ########## @@ -218,10 +218,14 @@ public ExprNodeDesc visitCall(RexCall call) { return args.get(0); } else if (call.isA(SqlKind.AND)) { // Make sure AND is flattened (we may have nested ANDs due to SearchTransformer conversion above) - return ExprNodeDescUtils.and(args); + // Flattening/deduplication may collapse the operand list; + // AND require at least two operands (GenericUDFOPAnd#initialize enforces this), so unwrapping trivial cases. + return unwrapSingleOperand(ExprNodeDescUtils.and(args)); } else if (call.isA(SqlKind.OR)) { // Make sure OR is flattened (we may have nested ORs due to SearchTransformer conversion above) - return ExprNodeDescUtils.or(args); + // Flattening/deduplication may collapse the operand list; + // OR require at least two operands (GenericUDFOPOr#initialize enforces this), so unwrapping trivial cases. + return unwrapSingleOperand(ExprNodeDescUtils.or(args)); Review Comment: Once we adopt the patch that makes the fix inside `ExprNodeDescUtils` all the comments about unrapping should become part of the Javadoc of the respective methods. ########## ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/translator/TestExprNodeConverter.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.translator; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.hadoop.hive.ql.optimizer.calcite.HiveTypeSystemImpl; +import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; +import org.junit.Test; + +import java.math.BigDecimal; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +public class TestExprNodeConverter { + + private final RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl(new HiveTypeSystemImpl()); + private final RexBuilder rexBuilder = new RexBuilder(typeFactory); + private final RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + private final RelDataType rowType = typeFactory.builder().add("a", intType).add("b", intType).build(); + + private ExprNodeDesc convert(RexNode node) { + return node.accept(new ExprNodeConverter("t", rowType, Collections.emptySet(), typeFactory)); + } + + private RexNode intLiteral(int value) { + return rexBuilder.makeExactLiteral(BigDecimal.valueOf(value), intType); + } + + private RexNode equalsCall(RexNode left, RexNode right) { + return rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, left, right); + } + + @Test + public void testAndWithDuplicateConstantOperandsIsUnwrapped() { + RexNode eq = equalsCall(intLiteral(1), intLiteral(1)); + ExprNodeDesc result = convert(rexBuilder.makeCall(SqlStdOperatorTable.AND, eq, eq)); + + List<ExprNodeDesc> resultNodes = result.getChildren(); + assertEquals(2, resultNodes.size()); + assertEquals("Const int 1", resultNodes.get(0).toString()); + assertEquals("Const int 1", resultNodes.get(1).toString()); + } + + @Test + public void testAndWithDuplicateColumnOperandsIsUnwrapped() { Review Comment: The name is bit misleading. The operands are duplicate but they are not columns; they are equality expressions. Compared to the previous test case just above not sure if it increases coverage. ########## ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestHiveRexExecutor.java: ########## @@ -0,0 +1,76 @@ +/* + * 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; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlBinaryOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.Test; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class TestHiveRexExecutor { + + private final RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl(new HiveTypeSystemImpl()); + private final RexBuilder rexBuilder = new RexBuilder(typeFactory); + private final RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + + private RexNode intLiteral(int value) { + return rexBuilder.makeExactLiteral(BigDecimal.valueOf(value), intType); + } + + private RexNode equalsCall(RexNode left, RexNode right) { + return rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, left, right); + } + + @Test + public void testReduceAndWithDuplicateConstantOperands() { + assertSpecifiedOperatorWithDuplicateConstantOperands(SqlStdOperatorTable.AND); + } + + @Test + public void testReduceOrWithDuplicateConstantOperands() { + assertSpecifiedOperatorWithDuplicateConstantOperands(SqlStdOperatorTable.OR); + } + + private void assertSpecifiedOperatorWithDuplicateConstantOperands(SqlBinaryOperator operator) { Review Comment: This very specific assert method is unlikely to be used by anyone else writing tests in this class in the future. Generalizing it a bit and taking the expression under test outside of the method would increase the likelihood of others taking advantage of it. ```suggestion private void assertReduce(String expected, RexNode node) { ``` -- 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]
