You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/03/13 11:01:03 UTC
[GitHub] [calcite] devozerov opened a new pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
devozerov opened a new pull request #2371:
URL: https://github.com/apache/calcite/pull/2371
See https://issues.apache.org/jira/browse/CALCITE-4535 for more information.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593766808
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +532,28 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ try {
+ statement.execute("drop " + objectType + " s.o");
+ fail("expected error");
Review comment:
I suggest `assertThrows` since:
1) It better structures the code (it is easier to see which block is expected to fail)
2) It has `reason` parameter. Please add proper reasoning that explains why `expected error`.
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +532,28 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ try {
+ statement.execute("drop " + objectType + " s.o");
+ fail("expected error");
Review comment:
I suggest `Assertions.assertThrows` since:
1) It better structures the code (it is easier to see which block is expected to fail)
2) It has `reason` parameter. Please add proper reasoning that explains why `expected error`.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593772431
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
```suggestion
() -> "expected error from 'drop " + objectType + " s.o' because it doesn't exist");
```
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593860128
##########
File path: server/src/main/java/org/apache/calcite/server/ServerDdlExecutor.java
##########
@@ -281,41 +281,54 @@ public void execute(SqlCreateFunction create,
* {@code DROP VIEW} commands. */
public void execute(SqlDropObject drop,
CalcitePrepare.Context context) {
- final List<String> path = context.getDefaultSchemaPath();
- CalciteSchema schema = context.getRootSchema();
- for (String p : path) {
- schema = schema.getSubSchema(p, true);
- }
- final boolean existed;
+ final Pair<CalciteSchema, String> pair = schema(context, false, drop.name);
+ CalciteSchema schema = pair.left;
+ String objectName = pair.right;
+ assert objectName != null;
+
+ boolean existed;
switch (drop.getKind()) {
case DROP_TABLE:
case DROP_MATERIALIZED_VIEW:
- existed = schema.removeTable(drop.name.getSimple());
- if (!existed && !drop.ifExists) {
+ Table materializedView = schema != null && drop.getKind() == SqlKind.DROP_MATERIALIZED_VIEW
+ ? schema.plus().getTable(objectName) : null;
+
+ existed = schema != null && schema.removeTable(objectName);
+ if (existed) {
+ if (materializedView != null) {
+ if (materializedView instanceof Wrapper) {
+ ((Wrapper) materializedView).maybeUnwrap(MaterializationKey.class)
+ .ifPresent(materializationKey -> {
+ MaterializationService.instance()
+ .removeMaterialization(materializationKey);
+ });
+ }
+ }
+ } else if (!drop.ifExists) {
throw SqlUtil.newContextException(drop.name.getParserPosition(),
- RESOURCE.tableNotFound(drop.name.getSimple()));
+ RESOURCE.tableNotFound(objectName));
}
break;
case DROP_VIEW:
// Not quite right: removes any other functions with the same name
- existed = schema.removeFunction(drop.name.getSimple());
+ existed = schema != null && schema.removeFunction(objectName);
Review comment:
Refactored. Note that for every object type we throw an exception with a different error message.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593808777
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
>the parent container doesn’t exist.
That is confusing. Test name says "schema does not exist", and assertion `reason` says "object does not exist".
Did you intend to add tests for the case when schema exists, yet the object is missing?
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593783912
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
Formally, the error is caused by the non-existent object, not the non-existent schema, this is why we check for the specific object type in the error message.
We may reconsider what is the real cause - either a non-existing target object or a non-existing parent object - but this is not a concert of this PR.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593853669
##########
File path: server/src/main/java/org/apache/calcite/server/ServerDdlExecutor.java
##########
@@ -281,41 +281,54 @@ public void execute(SqlCreateFunction create,
* {@code DROP VIEW} commands. */
public void execute(SqlDropObject drop,
CalcitePrepare.Context context) {
- final List<String> path = context.getDefaultSchemaPath();
- CalciteSchema schema = context.getRootSchema();
- for (String p : path) {
- schema = schema.getSubSchema(p, true);
- }
- final boolean existed;
+ final Pair<CalciteSchema, String> pair = schema(context, false, drop.name);
+ CalciteSchema schema = pair.left;
+ String objectName = pair.right;
+ assert objectName != null;
+
+ boolean existed;
switch (drop.getKind()) {
case DROP_TABLE:
case DROP_MATERIALIZED_VIEW:
- existed = schema.removeTable(drop.name.getSimple());
- if (!existed && !drop.ifExists) {
+ Table materializedView = schema != null && drop.getKind() == SqlKind.DROP_MATERIALIZED_VIEW
+ ? schema.plus().getTable(objectName) : null;
+
+ existed = schema != null && schema.removeTable(objectName);
+ if (existed) {
+ if (materializedView != null) {
Review comment:
There shouldn't be so many `if` statements nested.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593772431
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
```suggestion
() -> "expected error from 'drop " + objectType + " s.o' because it doesn't exist (because the schema does not exist)");
```
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593792209
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
If you mean `because object s.o` does not exist, then please ajdust the method name accordingly
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593783912
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
Formally, the error is caused by the non-existent object, not the non-existent schema. This is why we check for the specific object type in the error message.
We may reconsider what is the real cause - either a non-existing target object or a non-existing parent object - but this is not a concert of this PR.
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593771066
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +532,28 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ try {
+ statement.execute("drop " + objectType + " s.o");
+ fail("expected error");
Review comment:
Reworked to `assertThrows`.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593856461
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
Review comment:
Doesnt ---> not ?
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593859186
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
The description of the test case is not necessarily the same as the error message. The test case checks the behavior of the `DROP` command when the schema doesn't exist. If the schema doesn't exist, any object inside the schema doesn't exist either. Currently, Apache Calcite throws errors like `table not found` for such cases. Therefore, the current assertion message is correct - it expresses the same problem as we show to the user.
As I mentioned before, perhaps this is not the correct semantics. For example, Postgres will throw `schema not found`, which I think is a better error description. But this is not the concern of the given PR.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593856073
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
same question，+1
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593792209
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
If you mean `because object s.o` does not exist, then please adjust the method name accordingly
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593854675
##########
File path: server/src/main/java/org/apache/calcite/server/ServerDdlExecutor.java
##########
@@ -281,41 +281,54 @@ public void execute(SqlCreateFunction create,
* {@code DROP VIEW} commands. */
public void execute(SqlDropObject drop,
CalcitePrepare.Context context) {
- final List<String> path = context.getDefaultSchemaPath();
- CalciteSchema schema = context.getRootSchema();
- for (String p : path) {
- schema = schema.getSubSchema(p, true);
- }
- final boolean existed;
+ final Pair<CalciteSchema, String> pair = schema(context, false, drop.name);
+ CalciteSchema schema = pair.left;
+ String objectName = pair.right;
+ assert objectName != null;
+
+ boolean existed;
switch (drop.getKind()) {
case DROP_TABLE:
case DROP_MATERIALIZED_VIEW:
- existed = schema.removeTable(drop.name.getSimple());
- if (!existed && !drop.ifExists) {
+ Table materializedView = schema != null && drop.getKind() == SqlKind.DROP_MATERIALIZED_VIEW
+ ? schema.plus().getTable(objectName) : null;
+
+ existed = schema != null && schema.removeTable(objectName);
+ if (existed) {
+ if (materializedView != null) {
+ if (materializedView instanceof Wrapper) {
+ ((Wrapper) materializedView).maybeUnwrap(MaterializationKey.class)
+ .ifPresent(materializationKey -> {
+ MaterializationService.instance()
+ .removeMaterialization(materializationKey);
+ });
+ }
+ }
+ } else if (!drop.ifExists) {
throw SqlUtil.newContextException(drop.name.getParserPosition(),
- RESOURCE.tableNotFound(drop.name.getSimple()));
+ RESOURCE.tableNotFound(objectName));
}
break;
case DROP_VIEW:
// Not quite right: removes any other functions with the same name
- existed = schema.removeFunction(drop.name.getSimple());
+ existed = schema != null && schema.removeFunction(objectName);
Review comment:
`schema != null` could precondition.
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593859186
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
The description of the test case is not necessarily the same as the error message. The test case checks the behavior of the `DROP` command when the schema doesn't exist. If the schema doesn't exist, any object inside the schema doesn't exist either. Currently, Apache Calcite throws errors like `table not found` for such cases. Therefore, the current assertion message is correct - it expresses the problem in the same way as we it show to the user.
As I mentioned before, perhaps this is not the correct semantics. For example, Postgres will throw `schema not found`, which I think is a better error description. But this is not the concern of the given PR.
----------------------------------------------------------------
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
[GitHub] [calcite] asfgit closed pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2371:
URL: https://github.com/apache/calcite/pull/2371
--
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593792954
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
Method name already reflects the purpose - it checks behaviour of the DROP command when it is executed with the qualified object name and the parent container doesn’t exist.
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593783912
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
Formally, the error is caused by the non-existent object, not the non-existent schema. This is why we check for the specific object type in the error message.
We may reconsider what is the real cause - either a non-existing target object or a non-existing parent object - but this is not a concern of this PR.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593772431
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -532,4 +533,26 @@ static Connection connect() throws SQLException {
}
}
}
+
+ @Test public void testDropWithFullyQualifiedNameWhenSchemaDoesntExist() throws Exception {
+ try (Connection c = connect();
+ Statement s = c.createStatement()) {
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "schema", "Schema");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "table", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "materialized view", "Table");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "view", "View");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "type", "Type");
+ checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(s, "function", "Function");
+ }
+ }
+
+ private void checkDropWithFullyQualifiedNameWhenSchemaDoesntExist(
+ Statement statement, String objectType, String objectTypeInErrorMessage) throws Exception {
+ SQLException e = assertThrows(SQLException.class, () ->
+ statement.execute("drop " + objectType + " s.o"),
+ "expected error because the object doesn't exist");
Review comment:
```suggestion
"expected error from 'drop .. s.o' because schema s doesn't exist");
```
----------------------------------------------------------------
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
[GitHub] [calcite] devozerov commented on a change in pull request #2371: [CALCITE-4535] ServerDdlExecutor cannot execute DROP commands with qualified object names (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2371:
URL: https://github.com/apache/calcite/pull/2371#discussion_r593860042
##########
File path: server/src/main/java/org/apache/calcite/server/ServerDdlExecutor.java
##########
@@ -281,41 +281,54 @@ public void execute(SqlCreateFunction create,
* {@code DROP VIEW} commands. */
public void execute(SqlDropObject drop,
CalcitePrepare.Context context) {
- final List<String> path = context.getDefaultSchemaPath();
- CalciteSchema schema = context.getRootSchema();
- for (String p : path) {
- schema = schema.getSubSchema(p, true);
- }
- final boolean existed;
+ final Pair<CalciteSchema, String> pair = schema(context, false, drop.name);
+ CalciteSchema schema = pair.left;
+ String objectName = pair.right;
+ assert objectName != null;
+
+ boolean existed;
switch (drop.getKind()) {
case DROP_TABLE:
case DROP_MATERIALIZED_VIEW:
- existed = schema.removeTable(drop.name.getSimple());
- if (!existed && !drop.ifExists) {
+ Table materializedView = schema != null && drop.getKind() == SqlKind.DROP_MATERIALIZED_VIEW
+ ? schema.plus().getTable(objectName) : null;
+
+ existed = schema != null && schema.removeTable(objectName);
+ if (existed) {
+ if (materializedView != null) {
Review comment:
Fixed, one of the `if` statements was redundant. Are there any guidelines on the maximum number of nested `if`-s?
----------------------------------------------------------------
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