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/11 17:59:42 UTC
[GitHub] [calcite] devozerov opened a new pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
devozerov opened a new pull request #2369:
URL: https://github.com/apache/calcite/pull/2369
See https://issues.apache.org/jira/browse/CALCITE-4533 for the details.
----------------------------------------------------------------
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] amaliujia commented on pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#issuecomment-797666598
LGTM thanks!
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593006225
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
@vlsi I do not see any value in it, since these asserts check not for the outcome of the DDL operation per see, but whether JDBC returned a `ResultSet` or not. Obviously, for all DDL statements, it would be `false`. The incorrect behavior is checked through an exception that would be thrown otherwise. E.g., if `CREATE OR REPLACE` schema does not re-create the schema, it would not be possible to create a table with the same name as the table that existed in the previous shcema.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593056300
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
> The introduced changes do not make the test more confusing
The introduced changes make it hard to understand the nature of the changes.
I do not ask you to refactor all the existing asserts in the test.
I just kindly ask you to use a better style for the newly added code.
The question from xy2953396112 highlights that it is not really clear what `true/false` means in this context (it is tempting to assume that `true/false` means success/failure which turns out to be wrong).
Thanks.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593006225
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
@vlsi I do not see any value in it, since these asserts check not for the outcome of the DDL operation per se, but whether JDBC command returned a `ResultSet` or not. Obviously, for all DDL statements, it would be `false`. The incorrect behavior is checked through an exception that would be thrown otherwise. E.g., if `CREATE OR REPLACE` schema does not re-create the schema, it would not be possible to create a table with the same name as the table that existed in the previous schema.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r592994744
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Vladimir, would you please add `reason` messages to the asserts so it is clear why given values are expected?
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593045926
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
I agree with these comments, but the goal of the ticket was to fix particular bugs, not refactor the test. The introduced changes do not make the test more confusing, since they follow the established approach which is used dozens of times in the same test class. We may refactor the test if needed in a separate issue.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593039868
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
> I do not see any value in it, since these asserts check not for the outcome of the DDL operation per se, but whether JDBC command returned a ResultSet or not
Frankly speaking, that is not really obvious from the code, even though I admit `statement.execute` is the very basic JDBC API.
I would suggest:
1) If the outcome of `execute` is not needed, then skip the assert. Just ignore the returned value.
2) If you want to assert something, use `reason` parameter or add a helper method like `assertExecutesWithoutResutlSet(s, "create schema if not exists s")`. Then the test code would be shorter, it would be easier to understand, and the failure messages could be way more readable. For instance, the helper method could throw errors like "statement produced resultset with columns ... while no resultset was expected".
----------------------------------------------------------------
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 edited a comment on pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#issuecomment-797285619
please fix commits.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593132261
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Comments in code get out of date extremely fast, so it is better to put them into the exception message.
What do you think of `assertDoesNotThrow` with a relevant `reason` 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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593039868
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
> I do not see any value in it, since these asserts check not for the outcome of the DDL operation per se, but whether JDBC command returned a ResultSet or not
Frankly speaking, that is not really obvious from the code, even though I admit `statement.execute` is the very basic JDBC API.
I would suggest:
1) If the outcome of `execute` is not needed, then skip the assert. Just ignore the returned value.
2) If you want to assert something, use `reason` method or add a helper method like `assertExecutesWithoutResutlSet(s, "create schema if not exists s")`. Then the test code would be shorter, it would be easier to understand, and the failure messages could be way more readable. For instance, the helper method could throw errors like "statement produced resultset with columns ... while no resultset was expected".
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593156845
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Makes sense, I update the tests to use `assertDoesNotThrow`.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593096896
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Makes sense. I removed these assertions completely, as they are unrelated to the fix. Also added comments to the tests.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r592953187
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Why not true?
Schema not created successfully?
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593131391
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -207,6 +215,12 @@ static Connection connect() throws SQLException {
assertThat(b, is(false));
x = s.executeUpdate("insert into t2 values (1, NULL)");
assertThat(x, is(1));
+
+ // REPLACE must re-create the table, which we verify through the
+ // subsequent INSERT command, which expects only one column in the table.
+ s.execute("create or replace table t2 (i int not null)");
+ x = s.executeUpdate("insert into t2 values (1)");
+ assertThat(x, is(1));
Review comment:
```suggestion
assertDoesNotThrow(() -> {
s.execute("create or replace table t2 (i int not null)");
s.executeUpdate("insert into t2 values (1)");
}, "test table t2 should have been successfully recreated with one column, and insert values(1) should pass");
```
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593045926
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
I agree with these comments, but the goal of the ticket was to fix particular bugs, not to refactor the test. The introduced changes do not make the test more confusing, since they follow the established approach which is used dozens of times in the same test class. We may refactor the test if needed in a separate issue.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593131391
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -207,6 +215,12 @@ static Connection connect() throws SQLException {
assertThat(b, is(false));
x = s.executeUpdate("insert into t2 values (1, NULL)");
assertThat(x, is(1));
+
+ // REPLACE must re-create the table, which we verify through the
+ // subsequent INSERT command, which expects only one column in the table.
+ s.execute("create or replace table t2 (i int not null)");
+ x = s.executeUpdate("insert into t2 values (1)");
+ assertThat(x, is(1));
Review comment:
```suggestion
assertDoesNotThrow(() -> {
s.execute("create or replace table t2 (i int not null)");
s.executeUpdate("insert into t2 values (1)");
}, () -> "test table t2 should have been successfully recreated with one column, and insert values(1) should pass");
```
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593004409
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
+ x = s.executeUpdate("insert into s.t values 2");
+ assertThat(x, is(1));
+
+ b = s.execute("create or replace schema s");
+ assertThat(b, is(false));
Review comment:
`false` means that the result is not a `ResultSet`
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
@xy2953396112 `false` means that the result is not a `ResultSet`
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r592751131
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
+ x = s.executeUpdate("insert into s.t values 2");
+ assertThat(x, is(1));
+
+ b = s.execute("create or replace schema s");
+ assertThat(b, is(false));
Review comment:
Should this be true as schema s can be replaced?
----------------------------------------------------------------
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] amaliujia merged pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #2369:
URL: https://github.com/apache/calcite/pull/2369
----------------------------------------------------------------
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 pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#issuecomment-797285619
squash commits?
----------------------------------------------------------------
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 pull request #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#issuecomment-797339816
@xy2953396112
> please fix commits.
Fixed.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593006225
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
@vlsi I do not see any value in it, since these asserts check not for the outcome of the DDL operation per se, but whether JDBC returned a `ResultSet` or not. Obviously, for all DDL statements, it would be `false`. The incorrect behavior is checked through an exception that would be thrown otherwise. E.g., if `CREATE OR REPLACE` schema does not re-create the schema, it would not be possible to create a table with the same name as the table that existed in the previous shcema.
----------------------------------------------------------------
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 #2369: [CALCITE-4533] Fix handling of REPLACE and IF NOT EXISTS keywords for CREATE TABLE/SCHEMA commands (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2369:
URL: https://github.com/apache/calcite/pull/2369#discussion_r593156845
##########
File path: server/src/test/java/org/apache/calcite/test/ServerTest.java
##########
@@ -127,6 +127,16 @@ static Connection connect() throws SQLException {
assertThat(r.getInt(1), is(1));
assertThat(r.next(), is(false));
}
+
+ b = s.execute("create schema if not exists s");
+ assertThat(b, is(false));
Review comment:
Makes sense, I updated the tests to use `assertDoesNotThrow`.
----------------------------------------------------------------
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