You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/09/27 07:55:24 UTC

[GitHub] [shardingsphere] dongzl opened a new pull request #7619: Parse SQLServer insert output clause.

dongzl opened a new pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619


   For #6478 .
   
   Changes proposed in this pull request:
   - Parse SQLServer insert output clause.
   - add unit test.
   


----------------------------------------------------------------
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] [shardingsphere] jingshanglu commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495565885



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       > @jingshanglu @tristaZero @dongzl Now, the refactor task of `Statement` just splits different dialect statements, `StatementContext` and `StatementAssert` are not splitted. When `Statement` is used by `StatemenContext` and `StatementAssert`, different dialect statements are processed uniformly through the `StatementHandler` now.
   > 
   > I was wondering if we need to split `StatemenContext` and `StatementAssert`, since `StatementHandler` seems to handle logic well already.
   
   you mean the two way? decide whether to assert or not according to DB type in  specific function, like some segment  only contained by SQLServer.




----------------------------------------------------------------
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] [shardingsphere] dongzl commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
dongzl commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495557641



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       > Mybe it is better to make assertions based on specific DB.How do you think?
   
   @jingshanglu , yes, it's a very good idea, but I suggest we can do it in another PR, do you think it?




----------------------------------------------------------------
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] [shardingsphere] dongzl commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
dongzl commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495568059



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       Hi @jingshanglu , @strongduanmu  I suggest we open a new issue to discuss this problem, This is a big code refactor job, we need discuss it with more people. :)




----------------------------------------------------------------
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] [shardingsphere] jingshanglu commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495558198



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       @dongzl @tristaZero @strongduanmu    We should discuss aboult parser test framework,one:first determin db type and then assert; two:assert all segment include in all db in `InsertStatementAssert.java`,and then decide whether to assert or not according to DB type.What do you think?




----------------------------------------------------------------
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] [shardingsphere] coveralls commented on pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#issuecomment-699608179


   ## Pull Request Test Coverage Report for [Build 15095](https://coveralls.io/builds/33748264)
   
   * **26** of **33**   **(78.79%)**  changed or added relevant lines in **3** files are covered.
   * **4075** unchanged lines in **1** file lost coverage.
   * Overall coverage increased (+**0.1%**) to **35.241%**
   
   ---
   
   |  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
   | :-----|--------------|--------|---: |
   | [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/dialect/statement/sqlserver/dml/SQLServerInsertStatement.java](https://coveralls.io/builds/33748264/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fdialect%2Fstatement%2Fsqlserver%2Fdml%2FSQLServerInsertStatement.java#L55) | 0 | 1 | 0.0%
   | [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/java/org/apache/shardingsphere/sql/parser/sqlserver/visitor/impl/SQLServerDMLVisitor.java](https://coveralls.io/builds/33748264/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-sqlserver%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsqlserver%2Fvisitor%2Fimpl%2FSQLServerDMLVisitor.java#L151) | 26 | 29 | 89.66%
   | [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/dialect/handler/dml/InsertStatementHandler.java](https://coveralls.io/builds/33748264/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fdialect%2Fhandler%2Fdml%2FInsertStatementHandler.java#L91) | 0 | 3 | 0.0%
   <!-- | **Total:** | **26** | **33** | **78.79%** | -->
   
   |  Files with Coverage Reduction | New Missed Lines | % |
   | :-----|--------------|--: |
   | [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/SQLServerStatementParser.java](https://coveralls.io/builds/33748264/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-sqlserver%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FSQLServerStatementParser.java#L4403) | 4075 | 30.53% |
   <!-- | **Total:** | **4075** |  | -->
   
   |  Totals | [![Coverage Status](https://coveralls.io/builds/33748264/badge)](https://coveralls.io/builds/33748264) |
   | :-- | --: |
   | Change from base [Build 15092](https://coveralls.io/builds/33747850): |  0.1% |
   | Covered Lines: | 36778 |
   | Relevant Lines: | 104362 |
   
   ---
   ##### 💛  - [Coveralls](https://coveralls.io)
   


----------------------------------------------------------------
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] [shardingsphere] tristaZero merged pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619


   


----------------------------------------------------------------
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] [shardingsphere] jingshanglu commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495565885



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       > @jingshanglu @tristaZero @dongzl Now, the refactor task of `Statement` just splits different dialect statements, `StatementContext` and `StatementAssert` are not splitted. When `Statement` is used by `StatemenContext` and `StatementAssert`, different dialect statements are processed uniformly through the `StatementHandler` now.
   > 
   > I was wondering if we need to split `StatemenContext` and `StatementAssert`, since `StatementHandler` seems to handle logic well already.
   
   you mean the second way? decide whether to assert or not according to DB type in  specific function, like some segment  only contained by SQLServer.




----------------------------------------------------------------
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] [shardingsphere] strongduanmu commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495562425



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       @jingshanglu @tristaZero @dongzl Now, the refactor task of `Statement` just splits different dialect statements, `StatementContext` and `StatementAssert` are not splitted. When `Statement` is used by `StatemenContext` and `StatementAssert`, different dialect statements are processed uniformly through the `StatementHandler` now.
   
   I was wondering if we need to split `StatemenContext` and `StatementAssert`, since `StatementHandler` seems to handle logic well already.




----------------------------------------------------------------
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] [shardingsphere] jingshanglu commented on a change in pull request #7619: Parse SQLServer insert output clause.

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7619:
URL: https://github.com/apache/shardingsphere/pull/7619#discussion_r495555593



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/asserts/statement/dml/impl/InsertStatementAssert.java
##########
@@ -59,6 +61,7 @@ public static void assertIs(final SQLCaseAssertContext assertContext, final Inse
         assertInsertSelectClause(assertContext, actual, expected);
         assertOnDuplicateKeyColumns(assertContext, actual, expected);
         assertWithClause(assertContext, actual, expected);
+        assertOutputClause(assertContext, actual, expected);

Review comment:
       Mybe it is better to make assertions based on specific DB.How do you think?




----------------------------------------------------------------
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