Repository: calcite Updated Branches: refs/heads/master bdb953fa0 -> 0cbd2a182
[CALCITE-1900] Detect cyclic views and give useful error message Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/0cbd2a18 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/0cbd2a18 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/0cbd2a18 Branch: refs/heads/master Commit: 0cbd2a1825768835dccce1b24f746bc968908c3a Parents: bdb953f Author: Julian Hyde <[email protected]> Authored: Mon Jul 24 08:41:43 2017 -0700 Committer: Julian Hyde <[email protected]> Committed: Mon Jul 24 20:19:12 2017 -0700 ---------------------------------------------------------------------- .../calcite/jdbc/CalciteConnectionImpl.java | 4 ++ .../org/apache/calcite/jdbc/CalcitePrepare.java | 21 +++++- .../apache/calcite/runtime/CalciteResource.java | 3 + .../java/org/apache/calcite/schema/Schemas.java | 46 ++++++++---- .../calcite/schema/impl/ViewTableMacro.java | 7 +- .../sql/validate/CyclicDefinitionException.java | 37 ++++++++++ .../sql/validate/IdentifierNamespace.java | 14 +++- .../calcite/runtime/CalciteResource.properties | 1 + .../java/org/apache/calcite/test/JdbcTest.java | 73 ++++++++++++++++++++ 9 files changed, 187 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java b/core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java index 97744a8..2df9253 100644 --- a/core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java +++ b/core/src/main/java/org/apache/calcite/jdbc/CalciteConnectionImpl.java @@ -471,6 +471,10 @@ abstract class CalciteConnectionImpl : ImmutableList.of(schemaName); } + public List<String> getObjectPath() { + return null; + } + public CalciteConnectionConfig config() { return connection.config(); } http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/jdbc/CalcitePrepare.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/jdbc/CalcitePrepare.java b/core/src/main/java/org/apache/calcite/jdbc/CalcitePrepare.java index 6ebd146..dee608d 100644 --- a/core/src/main/java/org/apache/calcite/jdbc/CalcitePrepare.java +++ b/core/src/main/java/org/apache/calcite/jdbc/CalcitePrepare.java @@ -41,6 +41,7 @@ import org.apache.calcite.runtime.Bindable; import org.apache.calcite.schema.Table; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.validate.CyclicDefinitionException; import org.apache.calcite.sql.validate.SqlValidator; import org.apache.calcite.util.ImmutableIntList; @@ -118,6 +119,13 @@ public interface CalcitePrepare { SparkHandler spark(); DataContext getDataContext(); + + /** Returns the path of the object being analyzed, or null. + * + * <p>The object is being analyzed is typically a view. If it is already + * being analyzed further up the stack, the view definition can be deduced + * to be cylic. */ + List<String> getObjectPath(); } /** Callback to register Spark as the main engine. */ @@ -175,7 +183,17 @@ public interface CalcitePrepare { } public static void push(Context context) { - THREAD_CONTEXT_STACK.get().push(context); + final Deque<Context> stack = THREAD_CONTEXT_STACK.get(); + final List<String> path = context.getObjectPath(); + if (path != null) { + for (Context context1 : stack) { + final List<String> path1 = context1.getObjectPath(); + if (path.equals(path1)) { + throw new CyclicDefinitionException(stack.size(), path); + } + } + } + stack.push(context); } public static Context peek() { @@ -210,6 +228,7 @@ public interface CalcitePrepare { throw new UnsupportedOperationException(); } } + } /** The result of parsing and validating a SQL query. */ http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/runtime/CalciteResource.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/runtime/CalciteResource.java b/core/src/main/java/org/apache/calcite/runtime/CalciteResource.java index 84f21e1..b8afa01 100644 --- a/core/src/main/java/org/apache/calcite/runtime/CalciteResource.java +++ b/core/src/main/java/org/apache/calcite/runtime/CalciteResource.java @@ -636,6 +636,9 @@ public interface CalciteResource { @BaseMessage("Cannot stream VALUES") ExInst<SqlValidatorException> cannotStreamValues(); + @BaseMessage("Cannot resolve ''{0}''; it references view ''{1}'', whose definition is cyclic") + ExInst<SqlValidatorException> cyclicDefinition(String id, String view); + @BaseMessage("Modifiable view must be based on a single table") ExInst<SqlValidatorException> modifiableViewMustBeBasedOnSingleTable(); http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/schema/Schemas.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/schema/Schemas.java b/core/src/main/java/org/apache/calcite/schema/Schemas.java index 69cc44e..bd671f9 100644 --- a/core/src/main/java/org/apache/calcite/schema/Schemas.java +++ b/core/src/main/java/org/apache/calcite/schema/Schemas.java @@ -289,7 +289,7 @@ public final class Schemas { final ImmutableMap<CalciteConnectionProperty, String> propValues = ImmutableMap.of(); final CalcitePrepare.Context context = - makeContext(connection, schema, schemaPath, propValues); + makeContext(connection, schema, schemaPath, null, propValues); CalcitePrepare.Dummy.push(context); try { return prepare.parse(context, sql); @@ -307,7 +307,7 @@ public final class Schemas { final ImmutableMap<CalciteConnectionProperty, String> propValues = ImmutableMap.of(); final CalcitePrepare.Context context = - makeContext(connection, schema, schemaPath, propValues); + makeContext(connection, schema, schemaPath, null, propValues); CalcitePrepare.Dummy.push(context); try { return prepare.convert(context, sql); @@ -319,15 +319,16 @@ public final class Schemas { /** Analyzes a view. For use within Calcite only. */ public static CalcitePrepare.AnalyzeViewResult analyzeView( final CalciteConnection connection, final CalciteSchema schema, - final List<String> schemaPath, final String sql, boolean fail) { + final List<String> schemaPath, final String viewSql, + List<String> viewPath, boolean fail) { final CalcitePrepare prepare = CalcitePrepare.DEFAULT_FACTORY.apply(); final ImmutableMap<CalciteConnectionProperty, String> propValues = ImmutableMap.of(); final CalcitePrepare.Context context = - makeContext(connection, schema, schemaPath, propValues); + makeContext(connection, schema, schemaPath, viewPath, propValues); CalcitePrepare.Dummy.push(context); try { - return prepare.analyzeView(context, sql, fail); + return prepare.analyzeView(context, viewSql, fail); } finally { CalcitePrepare.Dummy.pop(context); } @@ -340,7 +341,7 @@ public final class Schemas { final ImmutableMap<CalciteConnectionProperty, String> map) { final CalcitePrepare prepare = CalcitePrepare.DEFAULT_FACTORY.apply(); final CalcitePrepare.Context context = - makeContext(connection, schema, schemaPath, map); + makeContext(connection, schema, schemaPath, null, map); CalcitePrepare.Dummy.push(context); try { return prepare.prepareSql(context, CalcitePrepare.Query.of(sql), @@ -350,21 +351,33 @@ public final class Schemas { } } - public static CalcitePrepare.Context makeContext( - final CalciteConnection connection, final CalciteSchema schema, - final List<String> schemaPath, + /** + * Creates a context for the purposes of preparing a statement. + * + * @param connection Connection + * @param schema Schema + * @param schemaPath Path wherein to look for functions + * @param objectPath Path of the object being analyzed (usually a view), + * or null + * @param propValues Connection properties + * @return Context + */ + private static CalcitePrepare.Context makeContext( + CalciteConnection connection, CalciteSchema schema, + List<String> schemaPath, List<String> objectPath, final ImmutableMap<CalciteConnectionProperty, String> propValues) { if (connection == null) { final CalcitePrepare.Context context0 = CalcitePrepare.Dummy.peek(); final CalciteConnectionConfig config = mutate(context0.config(), propValues); return makeContext(config, context0.getTypeFactory(), - context0.getDataContext(), schema, schemaPath); + context0.getDataContext(), schema, schemaPath, objectPath); } else { final CalciteConnectionConfig config = mutate(connection.config(), propValues); return makeContext(config, connection.getTypeFactory(), - createDataContext(connection, schema.root().plus()), schema, schemaPath); + createDataContext(connection, schema.root().plus()), schema, + schemaPath, objectPath); } } @@ -383,7 +396,9 @@ public final class Schemas { final JavaTypeFactory typeFactory, final DataContext dataContext, final CalciteSchema schema, - final List<String> schemaPath) { + final List<String> schemaPath, final List<String> objectPath_) { + final ImmutableList<String> objectPath = + objectPath_ == null ? null : ImmutableList.copyOf(objectPath_); return new CalcitePrepare.Context() { public JavaTypeFactory getTypeFactory() { return typeFactory; @@ -402,6 +417,10 @@ public final class Schemas { return schemaPath; } + public List<String> getObjectPath() { + return objectPath; + } + public CalciteConnectionConfig config() { return connectionConfig; } @@ -542,8 +561,7 @@ public final class Schemas { private final SchemaPlus rootSchema; private final ImmutableMap<String, Object> map; - public DummyDataContext(CalciteConnection connection, - SchemaPlus rootSchema) { + DummyDataContext(CalciteConnection connection, SchemaPlus rootSchema) { this.connection = connection; this.rootSchema = rootSchema; this.map = http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/schema/impl/ViewTableMacro.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/schema/impl/ViewTableMacro.java b/core/src/main/java/org/apache/calcite/schema/impl/ViewTableMacro.java index 1b54e7a..5ded4c3 100644 --- a/core/src/main/java/org/apache/calcite/schema/impl/ViewTableMacro.java +++ b/core/src/main/java/org/apache/calcite/schema/impl/ViewTableMacro.java @@ -17,6 +17,7 @@ package org.apache.calcite.schema.impl; import org.apache.calcite.adapter.java.JavaTypeFactory; +import org.apache.calcite.jdbc.CalciteConnection; import org.apache.calcite.jdbc.CalcitePrepare; import org.apache.calcite.jdbc.CalciteSchema; import org.apache.calcite.rel.type.RelDataTypeImpl; @@ -67,9 +68,11 @@ public class ViewTableMacro implements TableMacro { } public TranslatableTable apply(List<Object> arguments) { + final CalciteConnection connection = + MaterializedViewTable.MATERIALIZATION_CONNECTION; CalcitePrepare.AnalyzeViewResult parsed = - Schemas.analyzeView(MaterializedViewTable.MATERIALIZATION_CONNECTION, - schema, schemaPath, viewSql, modifiable != null && modifiable); + Schemas.analyzeView(connection, schema, schemaPath, viewSql, viewPath, + modifiable != null && modifiable); final List<String> schemaPath1 = schemaPath != null ? schemaPath : schema.path(null); if ((modifiable == null || modifiable) http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/sql/validate/CyclicDefinitionException.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/validate/CyclicDefinitionException.java b/core/src/main/java/org/apache/calcite/sql/validate/CyclicDefinitionException.java new file mode 100644 index 0000000..73096ef --- /dev/null +++ b/core/src/main/java/org/apache/calcite/sql/validate/CyclicDefinitionException.java @@ -0,0 +1,37 @@ +/* + * 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.sql.validate; + +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Thrown when an object, such as a view, is found to have a cylic + * definition. */ +public class CyclicDefinitionException extends RuntimeException { + public final int depth; + public final List<String> path; + + /** Creates CyclicDefinitionException. */ + public CyclicDefinitionException(int depth, List<String> path) { + super("Cyclic object definition: " + path); + this.depth = depth; + this.path = ImmutableList.copyOf(path); + } +} + +// End CyclicDefinitionException.java http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespace.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespace.java b/core/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespace.java index 4f53fc0..e9e12e6 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespace.java +++ b/core/src/main/java/org/apache/calcite/sql/validate/IdentifierNamespace.java @@ -101,8 +101,18 @@ public class IdentifierNamespace extends AbstractNamespace { final SqlValidatorScope.ResolvedImpl resolved = new SqlValidatorScope.ResolvedImpl(); final List<String> names = SqlIdentifier.toStar(id.names); - parentScope.resolveTable(names, nameMatcher, - SqlValidatorScope.Path.EMPTY, resolved); + try { + parentScope.resolveTable(names, nameMatcher, + SqlValidatorScope.Path.EMPTY, resolved); + } catch (CyclicDefinitionException e) { + if (e.depth == 1) { + throw validator.newValidationError(id, + RESOURCE.cyclicDefinition(id.toString(), + SqlIdentifier.getString(e.path))); + } else { + throw new CyclicDefinitionException(e.depth - 1, e.path); + } + } SqlValidatorScope.Resolve previousResolve = null; if (resolved.count() == 1) { final SqlValidatorScope.Resolve resolve = http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties ---------------------------------------------------------------------- diff --git a/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties b/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties index 360fb6e..9461fa4 100644 --- a/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties +++ b/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties @@ -206,6 +206,7 @@ StreamMustGroupByMonotonic=Streaming aggregation requires at least one monotonic StreamMustOrderByMonotonic=Streaming ORDER BY must start with monotonic expression StreamSetOpInconsistentInputs=Set operator cannot combine streaming and non-streaming inputs CannotStreamValues=Cannot stream VALUES +CyclicDefinition=Cannot resolve ''{0}''; it references view ''{1}'', whose definition is cyclic ModifiableViewMustBeBasedOnSingleTable=Modifiable view must be based on a single table ModifiableViewMustHaveOnlyEqualityPredicates=Modifiable view must be predicated only on equality expressions MoreThanOneMappedColumn=View is not modifiable. More than one expression maps to column ''{0}'' of base table ''{1}'' http://git-wip-us.apache.org/repos/asf/calcite/blob/0cbd2a18/core/src/test/java/org/apache/calcite/test/JdbcTest.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 825babe..8496e42 100644 --- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java +++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java @@ -5534,6 +5534,79 @@ public class JdbcTest { + "name=Theodore\n"); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-1900";>[CALCITE-1900] + * Improve error message for cyclic views</a>. + * Previously got a {@link StackOverflowError}. */ + @Test public void testSelfReferentialView() throws Exception { + final CalciteAssert.AssertThat with = + modelWithView("select * from \"V\"", null); + with.query("select \"name\" from \"adhoc\".V") + .throws_("Cannot resolve 'adhoc.V'; it references view 'adhoc.V', " + + "whose definition is cyclic"); + } + + @Test public void testSelfReferentialView2() throws Exception { + final String model = "{\n" + + " version: '1.0',\n" + + " defaultSchema: 'adhoc',\n" + + " schemas: [ {\n" + + " name: 'adhoc',\n" + + " tables: [ {\n" + + " name: 'A',\n" + + " type: 'view',\n" + + " sql: " + + new JsonBuilder().toJsonString("select * from B") + "\n" + + " }, {\n" + + " name: 'B',\n" + + " type: 'view',\n" + + " sql: " + + new JsonBuilder().toJsonString("select * from C") + "\n" + + " }, {\n" + + " name: 'C',\n" + + " type: 'view',\n" + + " sql: " + + new JsonBuilder().toJsonString("select * from D, B") + "\n" + + " }, {\n" + + " name: 'D',\n" + + " type: 'view',\n" + + " sql: " + + new JsonBuilder().toJsonString( + "select * from (values (1, 'a')) as t(x, y)") + "\n" + + " } ]\n" + + " } ]\n" + + "}"; + final CalciteAssert.AssertThat with = + CalciteAssert.model(model); + // + // +-----+ + // V | + // A --> B --> C --> D + // + // A is not in a cycle, but depends on cyclic views + // B is cyclic + // C is cyclic + // D is not cyclic + with.query("select x from \"adhoc\".a") + .throws_("Cannot resolve 'adhoc.A'; it references view 'adhoc.B', " + + "whose definition is cyclic"); + with.query("select x from \"adhoc\".b") + .throws_("Cannot resolve 'adhoc.B'; it references view 'adhoc.B', " + + "whose definition is cyclic"); + // as previous, but implicit schema + with.query("select x from b") + .throws_("Cannot resolve 'B'; it references view 'adhoc.B', " + + "whose definition is cyclic"); + with.query("select x from \"adhoc\".c") + .throws_("Cannot resolve 'adhoc.C'; it references view 'adhoc.C', " + + "whose definition is cyclic"); + with.query("select x from \"adhoc\".d") + .returns("X=1\n"); + with.query("select x from \"adhoc\".d except select x from \"adhoc\".a") + .throws_("Cannot resolve 'adhoc.A'; it references view 'adhoc.B', " + + "whose definition is cyclic"); + } + /** Tests saving query results into temporary tables, per * {@link org.apache.calcite.avatica.Handler.ResultSink}. */ @Test public void testAutomaticTemporaryTable() throws Exception {
