You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/04/20 14:01:38 UTC
[GitHub] [flink] KurtYoung commented on a change in pull request #11727: [FLINK-17106][table] Support create and drop view in Flink SQL
KurtYoung commented on a change in pull request #11727:
URL: https://github.com/apache/flink/pull/11727#discussion_r411395660
##########
File path: flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java
##########
@@ -142,7 +144,7 @@
"Unsupported SQL query! sqlUpdate() only accepts a single SQL statement of type " +
"INSERT, CREATE TABLE, DROP TABLE, ALTER TABLE, USE CATALOG, USE [CATALOG.]DATABASE, " +
"CREATE DATABASE, DROP DATABASE, ALTER DATABASE, CREATE FUNCTION, " +
- "DROP FUNCTION, ALTER FUNCTION, CREATE CATALOG.";
+ "DROP FUNCTION, ALTER FUNCTION, CREATE CATALOG, CREATE VIEW, DROP VIEW.";
private static final String UNSUPPORTED_QUERY_IN_EXECUTE_SQL_MSG =
Review comment:
I believe this also should be updated since you added create/drop view support to method `executeOperation`
##########
File path: flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java
##########
@@ -770,7 +803,7 @@ private TableResult executeOperation(Operation operation) {
} else if (operation instanceof ShowFunctionsOperation) {
return buildShowResult(listFunctions());
} else {
- throw new TableException(UNSUPPORTED_QUERY_IN_EXECUTE_SQL_MSG);
+ throw new TableException(UNSUPPORTED_QUERY_IN_SQL_UPDATE_MSG);
Review comment:
We should avoid to use `UNSUPPORTED_QUERY_IN_SQL_UPDATE_MSG` anymore since `sqlUpdate` will be deprecated soon.
##########
File path: flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/catalog/CatalogTableITCase.scala
##########
@@ -946,6 +946,485 @@ class CatalogTableITCase(isStreamingMode: Boolean) extends AbstractTestBase {
expectedProperty.put("k2", "b")
assertEquals(expectedProperty, database.getProperties)
}
+
Review comment:
Can you move most of these newly introduced tests to `TableEnvironmentTest`? I'm not sure why we need IT cases for testing view support, and I also don't get the idea why we put view related tests in this `CatalogTableITCase`
##########
File path: flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/catalog/CatalogTableITCase.scala
##########
@@ -946,6 +946,485 @@ class CatalogTableITCase(isStreamingMode: Boolean) extends AbstractTestBase {
expectedProperty.put("k2", "b")
assertEquals(expectedProperty, database.getProperties)
}
+
Review comment:
And please use `executeSql` instead of `sqlUpdate` if possible
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org