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/calcite.git
The following commit(s) were added to refs/heads/master by this push: new bfc842a [CALCITE-2677] Struct types with one field are not mapped correctly to Java Classes bfc842a is described below commit bfc842a043fc1fb0776d4d2ac633cf91acda3dc0 Author: Stamatis Zampetakis <zabe...@gmail.com> AuthorDate: Fri Nov 16 15:09:34 2018 +0100 [CALCITE-2677] Struct types with one field are not mapped correctly to Java Classes 1. Remove wrong unnesting of single field struct types from JavaTypeFactoryImpl and EnumUtils. 2. Refactor and move EnumUtils#javaRowClass method in PhysTypeImpl since it is used in only one place. 3. Add tests for JavaTypeFactoryImpl#getJavaClass and PhysType#fieldClass methods. 4. Modify EnumerableCollect to work always with rows represented as Object[], necessary for having MULTISET working after fixing CALCITE-2776. 5. Modify PhysType#convertTo to use JavaRowFormat instead of PhysType since the conversion is meant to only change the representation of rows in Java. 6. Disallow optimizations of the row format in the PhysType#convertTo method. 7. Implement SqlFunctions.slice method according to the specification in StdSqlOperatorTable.SLICE. 8. Add struct.iq test for reproducing CALCITE-2677 and CALCITE-2776. 9. Escape tests affected by CALCITE-2776. --- .../calcite/adapter/enumerable/EnumUtils.java | 9 --- .../adapter/enumerable/EnumerableAggregate.java | 2 +- .../adapter/enumerable/EnumerableCollect.java | 18 ++++- .../adapter/enumerable/EnumerableTableScan.java | 5 +- .../calcite/adapter/enumerable/PhysType.java | 13 ++- .../calcite/adapter/enumerable/PhysTypeImpl.java | 25 ++++-- .../apache/calcite/jdbc/JavaTypeFactoryImpl.java | 3 - .../org/apache/calcite/runtime/SqlFunctions.java | 6 +- .../src/main/java/org/apache/calcite/util/Bug.java | 5 ++ .../calcite/adapter/enumerable/PhysTypeTest.java | 72 +++++++++++++++++ .../apache/calcite/jdbc/JavaTypeFactoryTest.java | 94 ++++++++++++++++++++++ .../java/org/apache/calcite/test/JdbcTest.java | 2 + .../java/org/apache/calcite/test/QuidemTest.java | 2 + core/src/test/resources/sql/struct.iq | 37 +++++++++ 14 files changed, 266 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java index c97b2b2..ac4c0aa 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java @@ -80,15 +80,6 @@ public class EnumUtils { return clazz instanceof Class ? clazz : Object[].class; } - static Class javaRowClass( - JavaTypeFactory typeFactory, RelDataType type) { - if (type.isStruct() && type.getFieldCount() == 1) { - type = type.getFieldList().get(0).getType(); - } - final Type clazz = typeFactory.getJavaClass(type); - return clazz instanceof Class ? (Class) clazz : Object[].class; - } - static List<Type> fieldTypes( final JavaTypeFactory typeFactory, final List<? extends RelDataType> inputTypes) { diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregate.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregate.java index aa33685..988e753 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregate.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableAggregate.java @@ -406,7 +406,7 @@ public class EnumerableAggregate extends Aggregate implements EnumerableRel { Expressions.return_( null, Expressions.call( - inputPhysType.convertTo(childExp, physType), + inputPhysType.convertTo(childExp, physType.getFormat()), BuiltInMethod.DISTINCT.method, Expressions.<Expression>list() .appendIfNotNull(physType.comparer())))); diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCollect.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCollect.java index 59c2776..f89fec0 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCollect.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableCollect.java @@ -43,6 +43,8 @@ public class EnumerableCollect extends Collect implements EnumerableRel { public Result implement(EnumerableRelImplementor implementor, Prefer pref) { final BlockBuilder builder = new BlockBuilder(); final EnumerableRel child = (EnumerableRel) getInput(); + // REVIEW zabetak January 7, 2019: Even if we ask the implementor to provide a result + // where records are represented as arrays (Prefer.ARRAY) this may not be respected. final Result result = implementor.visitChild(this, 0, child, Prefer.ARRAY); final PhysType physType = PhysTypeImpl.of( @@ -50,14 +52,24 @@ public class EnumerableCollect extends Collect implements EnumerableRel { getRowType(), JavaRowFormat.LIST); - // final Enumerable<Employee> child = <<child adapter>>; - // final List<Employee> list = child.toList(); + // final Enumerable child = <<child adapter>>; + // final Enumerable<Object[]> converted = child.select(<<conversion code>>); + // final List<Object[]> list = converted.toList(); Expression child_ = builder.append( "child", result.block); + // In the internal representation of multisets , every element must be a record. In case the + // result above is a scalar type we have to wrap it around a physical type capable of + // representing records. For this reason the following conversion is necessary. + // REVIEW zabetak January 7, 2019: If we can ensure that the input to this operator + // has the correct physical type (e.g., respecting the Prefer.ARRAY above) then this conversion + // can be removed. + Expression conv_ = + builder.append( + "converted", result.physType.convertTo(child_, JavaRowFormat.ARRAY)); Expression list_ = builder.append("list", - Expressions.call(child_, + Expressions.call(conv_, BuiltInMethod.ENUMERABLE_TO_LIST.method)); builder.add( diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableTableScan.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableTableScan.java index 78826a1..18dcee1 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableTableScan.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableTableScan.java @@ -203,10 +203,7 @@ public class EnumerableTableScan typeFactory, relFieldType.getComponentType(), JavaRowFormat.CUSTOM); final MethodCallExpression e2 = Expressions.call(BuiltInMethod.AS_ENUMERABLE2.method, e); - final RelDataType dummyType = this.rowType; - final Expression e3 = - elementPhysType.convertTo(e2, - PhysTypeImpl.of(typeFactory, dummyType, JavaRowFormat.LIST)); + final Expression e3 = elementPhysType.convertTo(e2, JavaRowFormat.LIST); return Expressions.call(e3, BuiltInMethod.ENUMERABLE_TO_LIST.method); default: return e; diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysType.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysType.java index f44ce9e..4381d32 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysType.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysType.java @@ -198,8 +198,19 @@ public interface PhysType { PhysType makeNullable(boolean nullable); /** Converts an enumerable of this physical type to an enumerable that uses a - * given physical type for its rows. */ + * given physical type for its rows. + * + * @deprecated As of 1.19, use {@link #convertTo(Expression, JavaRowFormat)}. + * The use of PhysType as a second parameter is misleading since only the row + * format of the expression is affected by the conversion. Moreover it requires + * to have at hand a PhysType object which is not really necessary for achieving + * the desired result.*/ + @Deprecated Expression convertTo(Expression expression, PhysType targetPhysType); + + /** Converts an enumerable of this physical type to an enumerable that uses + * the <code>targetFormat</code> for representing its rows. */ + Expression convertTo(Expression expression, JavaRowFormat targetFormat); } // End PhysType.java diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysTypeImpl.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysTypeImpl.java index 7464a27..790f441 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysTypeImpl.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/PhysTypeImpl.java @@ -48,7 +48,6 @@ import java.util.ArrayList; import java.util.Comparator; import java.util.List; -import static org.apache.calcite.adapter.enumerable.EnumUtils.javaRowClass; import static org.apache.calcite.adapter.enumerable.EnumUtils.overridingMethodDecl; /** Implementation of {@link PhysType}. */ @@ -70,7 +69,8 @@ public class PhysTypeImpl implements PhysType { this.javaRowClass = javaRowClass; this.format = format; for (RelDataTypeField field : rowType.getFieldList()) { - fieldClasses.add(javaRowClass(typeFactory, field.getType())); + Type fieldType = typeFactory.getJavaClass(field.getType()); + fieldClasses.add(fieldType instanceof Class ? (Class) fieldType : Object[].class); } } @@ -236,15 +236,30 @@ public class PhysTypeImpl implements PhysType { } public Expression convertTo(Expression exp, PhysType targetPhysType) { - final JavaRowFormat targetFormat = targetPhysType.getFormat(); + return convertTo(exp, targetPhysType.getFormat()); + } + + public Expression convertTo(Expression exp, JavaRowFormat targetFormat) { if (format == targetFormat) { return exp; } final ParameterExpression o_ = Expressions.parameter(javaRowClass, "o"); final int fieldCount = rowType.getFieldCount(); - return Expressions.call(exp, BuiltInMethod.SELECT.method, - generateSelector(o_, Util.range(fieldCount), targetFormat)); + // The conversion must be strict so optimizations of the targetFormat should not be performed + // by the code that follows. If necessary the target format can be optimized before calling + // this method. + PhysType targetPhysType = PhysTypeImpl.of(typeFactory, rowType, targetFormat, false); + final Expression selector; + switch (targetPhysType.getFormat()) { + case SCALAR: + selector = Expressions.call(BuiltInMethod.IDENTITY_SELECTOR.method); + break; + default: + selector = Expressions.lambda(Function1.class, + targetPhysType.record(fieldReferences(o_, Util.range(fieldCount))), o_); + } + return Expressions.call(exp, BuiltInMethod.SELECT.method, selector); } public Pair<Expression, Expression> generateCollationKey( diff --git a/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java b/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java index 7b689dc..d51de1c 100644 --- a/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java +++ b/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java @@ -165,9 +165,6 @@ public class JavaTypeFactoryImpl JavaType javaType = (JavaType) type; return javaType.getJavaClass(); } - if (type.isStruct() && type.getFieldCount() == 1) { - return getJavaClass(type.getFieldList().get(0).getType()); - } if (type instanceof BasicSqlType || type instanceof IntervalSqlType) { switch (type.getSqlTypeName()) { case VARCHAR: diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java index 28f9bed..b475000 100644 --- a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java +++ b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java @@ -2096,7 +2096,11 @@ public class SqlFunctions { /** Support the SLICE function. */ public static List slice(List list) { - return list; + List result = new ArrayList(list.size()); + for (Object e : list) { + result.add(structAccess(e, 0, null)); + } + return result; } /** Support the ELEMENT function. */ diff --git a/core/src/main/java/org/apache/calcite/util/Bug.java b/core/src/main/java/org/apache/calcite/util/Bug.java index 7930dbc..3e69a2c 100644 --- a/core/src/main/java/org/apache/calcite/util/Bug.java +++ b/core/src/main/java/org/apache/calcite/util/Bug.java @@ -178,6 +178,11 @@ public abstract class Bug { * Several test case not passed in CalciteSqlOperatorTest.java</a> is fixed. */ public static final boolean CALCITE_2539_FIXED = false; + /** Whether + * <a href="https://issues.apache.org/jira/browse/CALCITE-2776">[CALCITE-2776] + * Wrong value when accessing struct types with one attribute</a> is fixed. */ + public static final boolean CALCITE_2776_FIXED = false; + /** * Use this to flag temporary code. */ diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java new file mode 100644 index 0000000..4023f3a --- /dev/null +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java @@ -0,0 +1,72 @@ +/* + * 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.calcite.adapter.enumerable; + +import org.apache.calcite.adapter.java.JavaTypeFactory; +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.type.SqlTypeName; + +import com.google.common.collect.ImmutableList; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +/** + * Test for {@link org.apache.calcite.adapter.enumerable.PhysTypeImpl}. + */ +public final class PhysTypeTest { + private static final JavaTypeFactory TYPE_FACTORY = new JavaTypeFactoryImpl(); + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testFieldClassOnColumnOfOneFieldStructType() { + RelDataType columnType = TYPE_FACTORY.createStructType( + ImmutableList.of(TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER)), + ImmutableList.of("intField")); + RelDataType rowType = TYPE_FACTORY.createStructType( + ImmutableList.of(columnType), + ImmutableList.of("structField")); + + PhysType rowPhysType = PhysTypeImpl.of(TYPE_FACTORY, rowType, JavaRowFormat.ARRAY); + assertEquals(Object[].class, rowPhysType.fieldClass(0)); + } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testFieldClassOnColumnOfTwoFieldStructType() { + RelDataType columnType = TYPE_FACTORY.createStructType( + ImmutableList.of( + TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER), + TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR)), + ImmutableList.of( + "intField", + "strField")); + RelDataType rowType = TYPE_FACTORY.createStructType( + ImmutableList.of(columnType), + ImmutableList.of("structField")); + + PhysType rowPhysType = PhysTypeImpl.of(TYPE_FACTORY, rowType, JavaRowFormat.ARRAY); + assertEquals(Object[].class, rowPhysType.fieldClass(0)); + } + +} + +// End PhysTypeTest.java diff --git a/core/src/test/java/org/apache/calcite/jdbc/JavaTypeFactoryTest.java b/core/src/test/java/org/apache/calcite/jdbc/JavaTypeFactoryTest.java new file mode 100644 index 0000000..cd0ba7a --- /dev/null +++ b/core/src/test/java/org/apache/calcite/jdbc/JavaTypeFactoryTest.java @@ -0,0 +1,94 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.linq4j.tree.Types; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.type.SqlTypeName; + +import com.google.common.collect.ImmutableList; + +import org.junit.Test; + +import java.lang.reflect.Type; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Test for {@link org.apache.calcite.jdbc.JavaTypeFactoryImpl}. + */ +public final class JavaTypeFactoryTest { + private static final JavaTypeFactoryImpl TYPE_FACTORY = new JavaTypeFactoryImpl(); + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testGetJavaClassWithOneFieldStructDataTypeV1() { + RelDataType structWithOneField = TYPE_FACTORY.createStructType(OneFieldStruct.class); + assertEquals(OneFieldStruct.class, TYPE_FACTORY.getJavaClass(structWithOneField)); + } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testGetJavaClassWithOneFieldStructDataTypeV2() { + RelDataType structWithOneField = TYPE_FACTORY.createStructType( + ImmutableList.of(TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER)), + ImmutableList.of("intField")); + assertRecordType(TYPE_FACTORY.getJavaClass(structWithOneField)); + } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testGetJavaClassWithTwoFieldsStructDataType() { + RelDataType structWithTwoFields = TYPE_FACTORY.createStructType(TwoFieldStruct.class); + assertEquals(TwoFieldStruct.class, TYPE_FACTORY.getJavaClass(structWithTwoFields)); + } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2677">[CALCITE-2677] + * Struct types with one field are not mapped correctly to Java Classes</a>. */ + @Test public void testGetJavaClassWithTwoFieldsStructDataTypeV2() { + RelDataType structWithTwoFields = TYPE_FACTORY.createStructType( + ImmutableList.of( + TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER), + TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR)), + ImmutableList.of("intField", "strField")); + assertRecordType(TYPE_FACTORY.getJavaClass(structWithTwoFields)); + } + + private void assertRecordType(Type actual) { + String errorMessage = + "Type {" + actual.getTypeName() + "} is not a subtype of Types.RecordType"; + assertTrue(errorMessage, actual instanceof Types.RecordType); + } + + /***/ + private static class OneFieldStruct { + public Integer intField; + } + + /***/ + private static class TwoFieldStruct { + public Integer intField; + public String strField; + } +} + +// End JavaTypeFactoryTest.java diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java b/core/src/test/java/org/apache/calcite/test/JdbcTest.java index 86482a4..e39a81a 100644 --- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java +++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java @@ -101,6 +101,7 @@ import com.google.common.collect.Multimap; import org.hamcrest.Matcher; import org.hsqldb.jdbcDriver; +import org.junit.Assume; import org.junit.Ignore; import org.junit.Test; @@ -1972,6 +1973,7 @@ public class JdbcTest { } @Test public void testMultisetQueryWithSingleColumn() { + Assume.assumeTrue("[CALCITE-2776]", Bug.CALCITE_2776_FIXED); CalciteAssert.hr() .query("select multiset(\n" + " select \"deptno\" from \"hr\".\"emps\") as a\n" diff --git a/core/src/test/java/org/apache/calcite/test/QuidemTest.java b/core/src/test/java/org/apache/calcite/test/QuidemTest.java index 77d7191..34087d3 100644 --- a/core/src/test/java/org/apache/calcite/test/QuidemTest.java +++ b/core/src/test/java/org/apache/calcite/test/QuidemTest.java @@ -85,6 +85,8 @@ public abstract class QuidemTest { return Bug.CALCITE_1045_FIXED; case "calcite1048": return Bug.CALCITE_1048_FIXED; + case "calcite2776": + return Bug.CALCITE_2776_FIXED; } return null; }; diff --git a/core/src/test/resources/sql/struct.iq b/core/src/test/resources/sql/struct.iq new file mode 100644 index 0000000..a62af30 --- /dev/null +++ b/core/src/test/resources/sql/struct.iq @@ -0,0 +1,37 @@ +# struct.iq - Queries involving structured types +# +# 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. +# +!use post +!set outputformat mysql + +!if (fixed.calcite2776) { +# [CALCITE-2677] Struct types with one field are not mapped correctly to Java Classes +select * from (values + (1, ROW(1)), + (2, ROW(2))) as v(id,struct); ++----+--------+ +| ID | STRUCT | ++----+--------+ +| 1 | {1} | +| 2 | {2} | ++----+--------+ +(2 rows) + +!ok +!} + +# End struct.iq