This is an automated email from the ASF dual-hosted git repository. libenchao pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push: new 5ee1b1ba2c [CALCITE-5553] `RelStructuredTypeFlattener` produces bad plan for single field struct 5ee1b1ba2c is described below commit 5ee1b1ba2c9bd347f098bca758fd3032541db5e9 Author: Andrew Pilloud <apill...@google.com> AuthorDate: Mon Mar 6 13:31:36 2023 -0800 [CALCITE-5553] `RelStructuredTypeFlattener` produces bad plan for single field struct Close apache/calcite#3092 --- .../sql2rel/RelStructuredTypeFlattener.java | 15 ++++++++++++-- .../apache/calcite/sql/test/SqlAdvisorTest.java | 1 + .../apache/calcite/test/SqlToRelConverterTest.java | 6 ++++++ .../apache/calcite/test/SqlToRelConverterTest.xml | 14 +++++++++++++ .../org/apache/calcite/test/catalog/Fixture.java | 3 +++ .../test/catalog/MockCatalogReaderSimple.java | 23 +++++++++++----------- 6 files changed, 48 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java b/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java index 3c3ec316ba..1d0570eb7a 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java @@ -337,8 +337,19 @@ public class RelStructuredTypeFlattener implements ReflectiveVisitor { for (RelNode input : inputs) { fieldCnt += input.getRowType().getFieldCount(); if (fieldCnt > fieldIdx) { - return getNewForOldRel(input).getRowType().getFieldList().size() - == input.getRowType().getFieldList().size(); + List<RelDataTypeField> newTypeFields = getNewForOldRel(input).getRowType().getFieldList(); + List<RelDataTypeField> inputTypeFields = input.getRowType().getFieldList(); + if (newTypeFields.size() != inputTypeFields.size()) { + return false; + } + // Ensure single field nested structs aren't flattened + for (int i = 0; i < newTypeFields.size(); ++i) { + if (newTypeFields.get(i).getType().isStruct() + != inputTypeFields.get(i).getType().isStruct()) { + return false; + } + } + return true; } } return false; diff --git a/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java b/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java index 7a2cd77358..e9443a0626 100644 --- a/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java +++ b/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java @@ -90,6 +90,7 @@ class SqlAdvisorTest extends SqlValidatorTestCase { "TABLE(CATALOG.SALES.EMPTY_PRODUCTS)", "TABLE(CATALOG.SALES.EMP_ADDRESS)", "TABLE(CATALOG.SALES.DEPT)", + "TABLE(CATALOG.SALES.DEPT_SINGLE)", "TABLE(CATALOG.SALES.DEPT_NESTED)", "TABLE(CATALOG.SALES.DEPT_NESTED_EXPANDED)", "TABLE(CATALOG.SALES.BONUS)", diff --git a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java index 60215c3e06..7541d68e27 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java +++ b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java @@ -3715,6 +3715,12 @@ class SqlToRelConverterTest extends SqlToRelTestBase { sql(sql).ok(); } + @Test void testNestedStructSingleFieldAccessWhere() { + final String sql = "select dn.skill\n" + + "from sales.dept_single dn WHERE dn.skill.type = ''"; + sql(sql).ok(); + } + @Test void testFunctionWithStructInput() { final String sql = "select json_type(skill)\n" + "from sales.dept_nested"; diff --git a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml index 1636bc7c17..25c1a3c302 100644 --- a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml @@ -4776,6 +4776,20 @@ from sales.dept_nested dn]]> <![CDATA[ LogicalProject(EXPR$0=[$2.OTHERS.A]) LogicalTableScan(table=[[CATALOG, SALES, DEPT_NESTED]]) +]]> + </Resource> + </TestCase> + <TestCase name="testNestedStructSingleFieldAccessWhere"> + <Resource name="sql"> + <![CDATA[select dn.skill +from sales.dept_single dn WHERE dn.skill.type = '']]> + </Resource> + <Resource name="plan"> + <![CDATA[ +LogicalProject(SKILL=[ROW($0)]) + LogicalFilter(condition=[=($0, '')]) + LogicalProject(TYPE=[$0.TYPE]) + LogicalTableScan(table=[[CATALOG, SALES, DEPT_SINGLE]]) ]]> </Resource> </TestCase> diff --git a/testkit/src/main/java/org/apache/calcite/test/catalog/Fixture.java b/testkit/src/main/java/org/apache/calcite/test/catalog/Fixture.java index 8a527b4b84..f1e97dc469 100644 --- a/testkit/src/main/java/org/apache/calcite/test/catalog/Fixture.java +++ b/testkit/src/main/java/org/apache/calcite/test/catalog/Fixture.java @@ -68,6 +68,9 @@ final class Fixture extends AbstractFixture { .build()) .kind(StructKind.PEEK_FIELDS_NO_EXPAND) .build(); + final RelDataType singleRecordType = typeFactory.builder() + .add("TYPE", varchar10Type) + .build(); final RelDataType abRecordType = typeFactory.builder() .add("A", varchar10Type) .add("B", varchar10Type) diff --git a/testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java b/testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java index 6171e69c7e..4e60734f72 100644 --- a/testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java +++ b/testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java @@ -158,6 +158,12 @@ public class MockCatalogReaderSimple extends MockCatalogReader { deptTable.addColumn("NAME", fixture.varchar10Type); registerTable(deptTable); + // Register "DEPT_SINGLE" table. + MockTable deptSingleTable = + MockTable.create(this, salesSchema, "DEPT_SINGLE", false, 4); + deptSingleTable.addColumn("SKILL", fixture.singleRecordType); + registerTable(deptSingleTable); + // Register "DEPT_NESTED" table. MockTable deptNestedTable = MockTable.create(this, salesSchema, "DEPT_NESTED", false, 4); @@ -291,13 +297,10 @@ public class MockCatalogReaderSimple extends MockCatalogReader { registerTable(suppliersTable); // Register "EMP_20" and "EMPNULLABLES_20 views. - // Same columns as "EMP" amd "EMPNULLABLES", - // but "DEPTNO" not visible and set to 20 by default - // and "SAL" is visible but must be greater than 1000, - // which is the equivalent of: + // Same columns as "EMP" amd "EMPNULLABLES", but "DEPTNO" not visible and set to 20 by default + // and "SAL" is visible but must be greater than 1000, which is the equivalent of: // SELECT EMPNO, ENAME, JOB, MGR, HIREDATE, SAL, COMM, SLACKER - // FROM EMP - // WHERE DEPTNO = 20 AND SAL > 1000 + // FROM EMP WHERE DEPTNO = 20 AND SAL > 1000 final ImmutableIntList m0 = ImmutableIntList.of(0, 1, 2, 3, 4, 5, 6, 8); MockTable emp20View = new MockViewTable(this, salesSchema.getCatalogName(), salesSchema.getName(), @@ -412,12 +415,8 @@ public class MockCatalogReaderSimple extends MockCatalogReader { registerTable(structNullableTypeTable); // Register "STRUCT.T_10" view. - // Same columns as "STRUCT.T", - // but "F0.C0" is set to 10 by default, - // which is the equivalent of: - // SELECT * - // FROM T - // WHERE F0.C0 = 10 + // Same columns as "STRUCT.T", but "F0.C0" is set to 10 by default, which is the equivalent of: + // SELECT * FROM T WHERE F0.C0 = 10 // This table uses MockViewTable which does not populate the constrained columns with default // values on INSERT. final ImmutableIntList m1 = ImmutableIntList.of(0, 1, 2, 3, 4, 5, 6, 7, 8);